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-24575: Rewrite WCS calculation for define-visits #281

Merged
merged 9 commits into from Aug 17, 2020
Merged

Conversation

timj
Copy link
Member

@timj timj commented Aug 12, 2020

Uses information from registry rather than retrieving a dataset from the datastore.

Depends on lsst/daf_butler#350

@timj timj force-pushed the tickets/DM-24575 branch 3 times, most recently from 43868c6 to 1f30374 Compare August 14, 2020 16:33
Only one of them included it so we ended up with inconsistent
collections usage.
Which matches the default for Butler -- this makes clear that
the script user is not doing something other than the default.
Previously the queryDataIds call was unconstrained and so would
return data that was not relevant to the instrument that we
have explicitly selected for this command.  We now also
pass in the default raw collection to match.
This makes it easier to use information that comes directly
from an ObservationInfo without having to go through an
intermediate visitInfo
This allows defineVisits to easily call the WCS constructor
without needing to go through a VisitInfo
Falls back to old scheme if the registry doesn't include
ra/dec

This requires that the compute routine can request the
Instrument class.
# Ask the raw formatter to create the relevant WCS
# This allows flips to be taken into account
instrument = self.getInstrument(exposure.instrument)
rawFormatter = instrument.getRawFormatter({"detector": detectorId})
Copy link
Member

Choose a reason for hiding this comment

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

I don't think there's much we can do about this now, but I'm vaguely worried that someday we'll swap in a new Formatter for some instrument because we start writing images out differently, and we'll have no way to connect a particular Formatter to a particular exposure. I suppose if we can put enough information into cameraGeom as discussed on Slack, that ceases to be a problem in at least this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose it is technically possible for new data to need a different formatter to older data. In theory datastore knows exactly which formatter to use since it's stored there. We haven't formally got any public method for asking datastore though.

Copy link
Member

@TallJimbo TallJimbo Aug 14, 2020

Choose a reason for hiding this comment

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

This problem potentially affects Datastore as well - if you have a new repo and ingest old raws into it with a new stack, you could be ingesting with the wrong Formatter if it's changed from the original.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, effectively we can't handle that. If the format changes (Say from FITS to HDF) we are going to have to require that the formatter knows how to deal with both.

@timj timj merged commit b33ccd1 into master Aug 17, 2020
@timj timj deleted the tickets/DM-24575 branch August 17, 2020 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants