Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-7477

Increase mask plane size to 32 bits

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Story Points:
      12
    • Sprint:
      DRP F17-1, DRP F17-2
    • Team:
      Data Release Production

      Description

      As per RFC-25, increase the Mask plane bit count from 16 to 32, which should hopefully just be (but probably won't be) a matter of modifying a typedef.

        Attachments

          Issue Links

            Activity

            Hide
            rhl Robert Lupton added a comment -

            I think we should get compression working before we go to 32 bits.

            Show
            rhl Robert Lupton added a comment - I think we should get compression working before we go to 32 bits.
            Hide
            swinbank John Swinbank added a comment -

            Agreed with Robert Lupton that Nate Lust can go ahead with this in the short term, with the caveat that it makes the upcoming compression WG (even) more urgent.

            Show
            swinbank John Swinbank added a comment - Agreed with Robert Lupton that Nate Lust can go ahead with this in the short term, with the caveat that it makes the upcoming compression WG (even) more urgent.
            Hide
            rowen Russell Owen added a comment -

            I see that the code on the ticket branch is switching from unsigned 16 bit to signed 32 bit, instead of unsigned as I expected. Why use signed?

            Show
            rowen Russell Owen added a comment - I see that the code on the ticket branch is switching from unsigned 16 bit to signed 32 bit, instead of unsigned as I expected. Why use signed?
            Hide
            jbosch Jim Bosch added a comment -

            I'm also a little concerned about switching to signed here; not only could that generate a ton of new compiler warnings, it's fairly unusual to do bitwise operations on signed integers in C/C++ (though not in Python, and hence better compatibility with bitwise operations in Python would be the only good reason I can think of to use signed - but I wasn't aware we hard a problem with that). As a side note, I believe you only formally get 31 usable bits in a signed 32-bit integer according to the standard (something I learned from Serge Monkewitz long ago), because using bitwise operations to change the sign is illegal.

            Show
            jbosch Jim Bosch added a comment - I'm also a little concerned about switching to signed here; not only could that generate a ton of new compiler warnings, it's fairly unusual to do bitwise operations on signed integers in C/C++ (though not in Python, and hence better compatibility with bitwise operations in Python would be the only good reason I can think of to use signed - but I wasn't aware we hard a problem with that). As a side note, I believe you only formally get 31 usable bits in a signed 32-bit integer according to the standard (something I learned from Serge Monkewitz long ago), because using bitwise operations to change the sign is illegal.
            Hide
            nlust Nate Lust added a comment -

            We went with signed because using unsigned 32 caused a chain of instantiations to be necessary (image base, afw table, etc). This brought up questions of code bloat (these instantiations were only needed to support masks) and of type names. Currently we are debating on what letter to use for just masks in python, this change would have the question extend further down the line to the above mentioned images, tables, etc, for multiple classes in c++.

            Show
            nlust Nate Lust added a comment - We went with signed because using unsigned 32 caused a chain of instantiations to be necessary (image base, afw table, etc). This brought up questions of code bloat (these instantiations were only needed to support masks) and of type names. Currently we are debating on what letter to use for just masks in python, this change would have the question extend further down the line to the above mentioned images, tables, etc, for multiple classes in c++.
            Hide
            jbosch Jim Bosch added a comment - - edited

            This brought up questions of code bloat (these instantiations were only needed to support masks)

            This seems like premature optimization. The Mask pixel type is our second-most-frequently-used pixel type (after float32); if we're concerned with code bloat due to image instantiations we should consider dropping some other pixel type we rarely use instead of contorting an important one.

            Show
            jbosch Jim Bosch added a comment - - edited This brought up questions of code bloat (these instantiations were only needed to support masks) This seems like premature optimization. The Mask pixel type is our second-most-frequently-used pixel type (after float32); if we're concerned with code bloat due to image instantiations we should consider dropping some other pixel type we rarely use instead of contorting an important one.
            Hide
            jbosch Jim Bosch added a comment -

            Summarizing some in-person conversations for posterity:

            • There is general agreement that unsigned would be better, everything else being equal, but that using signed won't really cause any serious problems.
            • The big reason using unsigned is problematic is because we need afw::table field types (both scalar and array) that match the Mask type. This is probably because we use afw::table persistence to store objects containing masks (probably just HeavyFootprints, but this has not been verified) in FITS binary tables. The u/nlust/DM-7477 represents a switch to uint32 that could be used to test this theory.
            • We will proceed with the proposal to use (signed) int32 for now, but make sure that it's done in such a way that any later changes to the mask type really would involve just a small, centralized change (in addition to addressing the afw::table persistence issue, of course).
            Show
            jbosch Jim Bosch added a comment - Summarizing some in-person conversations for posterity: There is general agreement that unsigned would be better, everything else being equal, but that using signed won't really cause any serious problems. The big reason using unsigned is problematic is because we need afw::table field types (both scalar and array) that match the Mask type. This is probably because we use afw::table persistence to store objects containing masks (probably just HeavyFootprints, but this has not been verified) in FITS binary tables. The u/nlust/ DM-7477 represents a switch to uint32 that could be used to test this theory. We will proceed with the proposal to use (signed) int32 for now, but make sure that it's done in such a way that any later changes to the mask type really would involve just a small, centralized change (in addition to addressing the afw::table persistence issue, of course).
            Hide
            jbosch Jim Bosch added a comment - - edited

            Some mild bad news: the above has reminded me that changing the mask pixel type will break our ability to read catalogs containing {{HeavyFootprint}}s produced with earlier versions of the pipeline unless we explicitly add backwards compatibility code. That needs to be done.

            We should test that we can continue to read old 16-bit Masks into 32-bit Masks as well, though I think it's possible that will Just Work.

            Show
            jbosch Jim Bosch added a comment - - edited Some mild bad news: the above has reminded me that changing the mask pixel type will break our ability to read catalogs containing {{HeavyFootprint}}s produced with earlier versions of the pipeline unless we explicitly add backwards compatibility code. That needs to be done. We should test that we can continue to read old 16-bit Masks into 32-bit Masks as well, though I think it's possible that will Just Work.
            Hide
            nlust Nate Lust added a comment -

            The work intended to be done on this ticket, as copied from the RFC, is the following:

            *Python will contain an alias for afwImage.MaskPixel to np.int32 (same as C++) making it easier to change in the future if we choose
            *The Python instantiation associated with int32 will be named MaskX
            *The default type constructed by the Mask() abc will be the int 32 instance so m = Mask() will just work
            *Accessing static methods on the MaskX object will be done as Mask[afwImage.MaskPixel].static_method()

            This nessecitated making the changes in afw to change the pixel type in C++, updating the python bindings and creating a python type alias, and adding legacy support for reading HeavyFootprints.

            The changes in the remaining packages are all related to updating the name of the Mask object in python.

            Show
            nlust Nate Lust added a comment - The work intended to be done on this ticket, as copied from the RFC, is the following: *Python will contain an alias for afwImage.MaskPixel to np.int32 (same as C++) making it easier to change in the future if we choose *The Python instantiation associated with int32 will be named MaskX *The default type constructed by the Mask() abc will be the int 32 instance so m = Mask() will just work *Accessing static methods on the MaskX object will be done as Mask [afwImage.MaskPixel] .static_method() This nessecitated making the changes in afw to change the pixel type in C++, updating the python bindings and creating a python type alias, and adding legacy support for reading HeavyFootprints. The changes in the remaining packages are all related to updating the name of the Mask object in python.
            Hide
            price Paul Price added a comment -

            This is great. The only change I would insist upon is that in python we need to be able to write Mask.staticMethod(...) instead of Mask[MaskPixel].staticMethod(...) — the python user shouldn't need to know that MaskPixel even exists. I've pushed some changes to branch u/price/DM-7477 of afw and utils that should allow this behaviour.

            Why MaskX instead of MaskI, when we use I for int32 in other contexts?

            Show
            price Paul Price added a comment - This is great. The only change I would insist upon is that in python we need to be able to write Mask.staticMethod(...) instead of Mask [MaskPixel] .staticMethod(...) — the python user shouldn't need to know that MaskPixel even exists. I've pushed some changes to branch u/price/ DM-7477 of afw and utils that should allow this behaviour. Why MaskX instead of MaskI , when we use I for int32 in other contexts?
            Hide
            price Paul Price added a comment -

            Oh, and we need a test for reading legacy HeavyFootprint.

            Show
            price Paul Price added a comment - Oh, and we need a test for reading legacy HeavyFootprint .
            Hide
            nlust Nate Lust added a comment -

            MaskX was chosen to be just a place holder, and decouple it from the underlying type, as they type is unimportant, and may change.

            Show
            nlust Nate Lust added a comment - MaskX was chosen to be just a place holder, and decouple it from the underlying type, as they type is unimportant, and may change.
            Hide
            nlust Nate Lust added a comment -

            merged all to master

            Show
            nlust Nate Lust added a comment - merged all to master
            Hide
            xiuqin Xiuqin Wu [X] (Inactive) added a comment -

            Nate LustCould you provide a FIST image with the 32bits mask? I want to test that Firefly could display the mask layer correctly. Thank you!

            Show
            xiuqin Xiuqin Wu [X] (Inactive) added a comment - Nate Lust Could you provide a FIST image with the 32bits mask? I want to test that Firefly could display the mask layer correctly. Thank you!

              People

              Assignee:
              nlust Nate Lust
              Reporter:
              jbosch Jim Bosch
              Reviewers:
              Paul Price
              Watchers:
              Jim Bosch, John Swinbank, Nate Lust, Paul Price, Robert Lupton, Russell Owen, Xiuqin Wu [X] (Inactive)
              Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: