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-25156: During 2to3 conversion ask Instrument for the curated calibrations #291

Merged
merged 5 commits into from Aug 26, 2020

Conversation

timj
Copy link
Member

@timj timj commented Aug 21, 2020

Do not use the config file since the instrument already knows which calibrations it will curate.

Copy link
Member

@kfindeisen kfindeisen left a comment

Choose a reason for hiding this comment

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

Looks good, but I'm a bit worried about the special-casing of camera. I'd like to understand what's going on there before signing off on this.

python/lsst/obs/base/_instrument.py Outdated Show resolved Hide resolved
python/lsst/obs/base/_instrument.py Outdated Show resolved Hide resolved
python/lsst/obs/base/_instrument.py Outdated Show resolved Hide resolved
python/lsst/obs/base/_instrument.py Outdated Show resolved Hide resolved
python/lsst/obs/base/_instrument.py Outdated Show resolved Hide resolved
python/lsst/obs/base/_instrument.py Outdated Show resolved Hide resolved
# Do not check the data packages for calibrations for now.
# Camera is a special dataset type that is also handled as a
# curated calibration.
return set(("camera",) + cls.standardCuratedDatasetTypes + cls.additionalCuratedDatasetTypes)
Copy link
Member

Choose a reason for hiding this comment

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

Why does camera not belong in standardCuratedDatasetTypes? Special-casing something like this seems like it could cause bugs later.

If the point is that these are the curated calibrations only for conversion purposes, but not for others, then the method/property name should reflect that.

Copy link
Member Author

Choose a reason for hiding this comment

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

because "standard" means "calibration data found in obs_x_data package following the standard organization". At some point versioned camera geometry will turn up in that form and when it does we will include it in the standard list. If you look at writeCuratedCalbrations you will see it does standard curated, additional curated, and camera as distinct operations.

Copy link
Member Author

Choose a reason for hiding this comment

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

As for the name, I am wanting the list of names returned here to match the list of dataset types that writeCuratedCalibrations will be writing.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, it was not at all clear that "standard organization of obs_*_data" was the distinction being made. I assume this is what "standard definitions" meant in the docstring?

If that's the key difference, then consider rewording the description of additionalCuratedDatasetTypes from "Curated dataset types specific to this particular instrument." to "Curated dataset types that do not follow the standard organization defined in obs_base." I think that would make it a bit clearer what is and is not proper usage of these two variables.

This required that the code to locate standard curated calibrations
be moved into a class method.
@timj
Copy link
Member Author

timj commented Aug 26, 2020

I tweaked the logic a little bit to only return the calibrations understood by the instrument (so defects is not in the list if defects aren't defined in a standard way for that instrument (of course a subclass can declare them as "additional" curated calibrations).

@timj timj merged commit 38a3e24 into master Aug 26, 2020
@timj timj deleted the tickets/DM-25156 branch August 26, 2020 17:41
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