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

Restructure image formatter relationships

    XMLWordPrintable

    Details

    • Story Points:
      2
    • Epic Link:
    • Team:
      Data Release Production
    • Urgent?:
      No

      Description

      While working on DM-28583, I noticed some structural problems with our FITS image formatters that should probably be out of scope for that ticket, but are worth fixing at some point. DM-28583 will take care of splitting FitsExposureFormatter into a base class and an Exposure specialization (it's really not good substitutability for the Image/Mask/MaskedImage specializations to inherit from the Exposure one), but that will still leave too much Exposure-only stuff (and MaskedImage-only stuff) in the base class.

      I also don't think we should be running astro_metadata_translator's fix_headers in this formatter - we absolutely should be in FitsRawExposureFormatter, but after we read (patched) raws and rewrite that (patched) metadata into downstream image-like datasets, we shouldn't need to patch those again on read. That will hopefully make it possible to just delegate directly to afw.image.ExposureFitsReader more often (I'm guessing this is why we can't use its readMetadata and instead write our own (expensive?) stripping here), and reduce confusing differences between what Butler does and what direct FITS I/O does. It also suggests that perhaps FitsRawExposureFormatter should not inherit from FitsExposureFormatter (or even the latter's new base class), which would IMO be a good idea for other reasons - the overall flow between their many methods is quite different, and maintaining inheritance obfuscates both cases.

      I'm also a little uncomfortable with the approach I'm (currently) taking on DM-28583 of adding the Reader instance as a Formatter instance attribute in some methods so downstream methods can use it, just because I think that's a bad pattern for how to share state between methods, but it seems to be the only one that really works without major changes to the method structure (maybe going down all the way to Formatter itself). So that issue may be out of scope for this ticket (and I'm not totally sure it's worth worrying about), but it's worth looking for opportunities to fix it as part of any refactor.

      Finally, we need to remember that we can't change the names or module paths of any of the concrete (most-derived) Formatter classes as part of any restructuring - those are already baked into data repositories that we don't want to break.

      (I'm assigning this to Arch as a "generic middleware ticket", not because I'm asking Tim Jenness to do it, let alone do it now.)

        Attachments

          Issue Links

            Activity

            Hide
            jbosch Jim Bosch added a comment - - edited

            One more related structural issue from DM-28583: obs_lsst's test_gen3.py now uses obs_lsst's custom readRawFitsHeader to get an ObservationInfo.  I think we need a new and consistent interface for extracting an ObservationInfo from a raw file, perhaps on the Instrument class so it can be customized.  I think that was once an ambition of astro_metadata_translator itself, but the need to read headers differently for different instruments to deal with e.g. INHERIT problems gets in the way of that, and the result is that we're doing slightly different things in slightly different places to get ObservationInfos from raws.

            For posterity: there was already a consistent interface for this on the Formatter classes, and that's all we need.  I just wasn't aware of it.

             

            Show
            jbosch Jim Bosch added a comment - - edited One more related structural issue from DM-28583 : obs_lsst 's test_gen3.py now uses obs_lsst's custom readRawFitsHeader to get an ObservationInfo .  I think we need a new and consistent interface for extracting an ObservationInfo from a raw file, perhaps on the Instrument class so it can be customized.  I think that was once an ambition of astro_metadata_translator itself, but the need to read headers differently for different instruments to deal with e.g. INHERIT problems gets in the way of that, and the result is that we're doing slightly different things in slightly different places to get ObservationInfos from raws . For posterity: there was already a consistent interface for this on the  Formatter classes, and that's all we need.  I just wasn't aware of it.  
            Hide
            jbosch Jim Bosch added a comment - - edited

            This is getting in the way of DM-29370, to the point where I started doing a lot of the work for this ticket on the branches for that one.  I'm going to split those commits off to this ticket's branches to get them tested, reviewed, and merged separately.

            I don't think this needs to depend on DM-26658 getting done first, though this work may generate some thoughts on what else DM-26658 could do.

            Show
            jbosch Jim Bosch added a comment - - edited This is getting in the way of DM-29370 , to the point where I started doing a lot of the work for this ticket on the branches for that one.  I'm going to split those commits off to this ticket's branches to get them tested, reviewed, and merged separately. I don't think this needs to depend on DM-26658 getting done first, though this work may generate some thoughts on what else DM-26658 could do.
            Hide
            jbosch Jim Bosch added a comment -

            Tim Jenness, can you take a look at this when you get a chance?  I'm pretty happy with how it turned out overall, though I have two specific comments on the PR already that I'm hoping you'll be able to respond to, in addition to the regular review.  I recommend looking commit-by-commit, but not thinking about code duplication concerns specifically until you see the final state.

            Most work is in obs_base: https://github.com/lsst/obs_base/pull/377/files

            With a tiny change in obs_lsst: https://github.com/lsst/obs_lsst/pull/309

            Jira links to a bunch of other git stuff just because I've been griping about this ticket in commit messages for a while.

            Show
            jbosch Jim Bosch added a comment - Tim Jenness , can you take a look at this when you get a chance?  I'm pretty happy with how it turned out overall, though I have two specific comments on the PR already that I'm hoping you'll be able to respond to, in addition to the regular review.  I recommend looking commit-by-commit, but not thinking about code duplication concerns specifically until you see the final state. Most work is in obs_base: https://github.com/lsst/obs_base/pull/377/files With a tiny change in obs_lsst: https://github.com/lsst/obs_lsst/pull/309 Jira links to a bunch of other git stuff just because I've been griping about this ticket in commit messages for a while.
            Hide
            jbosch Jim Bosch added a comment -

            Take this out of review for a bit more work; I found a few more things I wanted to here while working on DM-29370 downstream of this.

            Show
            jbosch Jim Bosch added a comment - Take this out of review for a bit more work; I found a few more things I wanted to here while working on DM-29370 downstream of this.
            Hide
            jbosch Jim Bosch added a comment -

            Ready for review again.  The problem with earlier commits having some code duplication that's resolved later is now gone, because the changes I made more recently just made it more useful to just squash those together.

            Show
            jbosch Jim Bosch added a comment - Ready for review again.  The problem with earlier commits having some code duplication that's resolved later is now gone, because the changes I made more recently just made it more useful to just squash those together.
            Hide
            tjenness Tim Jenness added a comment -

            I have some questions on the main PR. Mostly my confusion over when headers are and are not stripped.

            Show
            tjenness Tim Jenness added a comment - I have some questions on the main PR. Mostly my confusion over when headers are and are not stripped.

              People

              Assignee:
              jbosch Jim Bosch
              Reporter:
              jbosch Jim Bosch
              Reviewers:
              Tim Jenness
              Watchers:
              Jim Bosch, John Parejko, Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins Builds

                  No builds found.