Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-31924: implement new data ID packer and deprecate ExposureIdInfo #450

Merged
merged 1 commit into from Apr 27, 2023

Conversation

TallJimbo
Copy link
Member

@TallJimbo TallJimbo commented Apr 14, 2023

Checklist

  • ran Jenkins
  • added a release note for user-visible changes to doc/changes

@timj
Copy link
Member

timj commented Apr 14, 2023

A general comment for consideration, the Instrument base class is in pipe_base so adding functionality in the obs_base subclass that has no dependency on non-middleware code should be considered carefully. Using obs_base declares that other middleware users who do not want to use afw and camera geom etc, will have no interest in using these ID packers.

@TallJimbo
Copy link
Member Author

That's a very good point. I put it in obs_base because I was thinking "this is a hook that obs_ packages may override", but I don't think it needs afw or anything else not available in pipe_base.

@timj
Copy link
Member

timj commented Apr 14, 2023

Yes, I moved the core Instrument functionality to pipe_base because it seemed like we really needed some form of Instrument that could be general enough.

@TallJimbo
Copy link
Member Author

Ok, I've moved everything on this ticket but the deprecation of old code to pipe_base.

Copy link
Collaborator

@parejkoj parejkoj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay code removal!

The replacement, CatalogIdPacker, is very similar, but it also has the
ability to unpack Source/Object IDs fully, and that's not something we
could add to ExposureIdInfo without breaking backwards compatibility
in its constructor signature.  Moving to a new but very similar class
has two more smaller advantages:

- it makes the transition from registry-provided dimension packers to
  configurable dimension packers easier by fully swapping out the
  helper classes that make the dimension packers;

- it eliminates one case of too-overloaded "Exposure" nomenclature.

I've moved the replacement to meas_base to avoid introducing a
dependency (in either direction) between obs_base and skymap; I don't
think those dependencies would be a big problem, but it's nice to avoid
dependencies when you can, and meas_base already fits the requirements
for where we'd put the new class:

- It already depends on both afw and daf_butler.

- It already depends on skymap and obs_base (either order for these
  relationships would acceptable, as we can register configurables
  with a pex_config registry either when we define the registry or
  when we define a particular configurable).

- It is already a dependency of all Tasks that would use it (some of
  which live in meas_base).
@TallJimbo TallJimbo force-pushed the tickets/DM-31924 branch 2 times, most recently from 5aee9e0 to a184f7d Compare April 26, 2023 18:08
@TallJimbo TallJimbo merged commit fce3b03 into main Apr 27, 2023
3 checks passed
@TallJimbo TallJimbo deleted the tickets/DM-31924 branch April 27, 2023 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants