Skip to:

  1. Skip to Jira Navigation
  2. Skip to Side Navigation
  3. Skip to Main Content

Restructure image formatter relationships

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 to do it, let alone do it now.)

Checklist

Lucidchart Diagrams

Issue Matrix

hide

Activity

Show:

Tim Jenness 
May 5, 2021 at 12:11 AM

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

Jim Bosch 
May 4, 2021 at 3:37 AM

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.

Jim Bosch 
May 3, 2021 at 9:01 PM

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

Jim Bosch 
May 2, 2021 at 3:59 PM

, 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.

Jim Bosch 
May 1, 2021 at 6:28 PM
(edited)

This is getting in the way of , 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 getting done first, though this work may generate some thoughts on what else could do.

Done
Pinned fields
Click on the next to a field label to start pinning.

Details

Assignee

Reporter

Labels

Reviewers

Tim Jenness

Story Points

RubinTeam

Components

Parent

Checklist

Created February 7, 2021 at 4:59 PM
Updated May 9, 2021 at 6:10 PM
Resolved May 9, 2021 at 6:10 PM