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-24537: Add Isr calibration base class #135

Merged
merged 2 commits into from Apr 29, 2020
Merged

DM-24537: Add Isr calibration base class #135

merged 2 commits into from Apr 29, 2020

Conversation

czwa
Copy link
Contributor

@czwa czwa commented Apr 23, 2020

Add IsrCalib and IsrProvenance classes.
IsrCalib should be the base class used by calibration data.
IsrProvenance supplies a concrete example subclass that will allow
calibrations to store their inputs in a way that can be used to
reconstruct the calibration.

@czwa czwa requested a review from timj April 23, 2020 22:00
Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

I'm sorry but I have lots of questions. Hopefully they're all straightforward though. This is definitely something we want to do and I think fairly soon we are going to have to try switching Curve and Defects to this scheme to prove it works.

python/lsst/ip/isr/calibType.py Outdated Show resolved Hide resolved
python/lsst/ip/isr/calibType.py Outdated Show resolved Hide resolved
python/lsst/ip/isr/calibType.py Outdated Show resolved Hide resolved
python/lsst/ip/isr/calibType.py Outdated Show resolved Hide resolved
python/lsst/ip/isr/calibType.py Show resolved Hide resolved
python/lsst/ip/isr/calibType.py Outdated Show resolved Hide resolved
python/lsst/ip/isr/calibType.py Show resolved Hide resolved

Parameters
----------
registryName : `str`, optional
Copy link
Member

Choose a reason for hiding this comment

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

What is a registry name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had intended this to be some way to identify the Butler repository that constructed the calibration, to ensure all the dataIds recorded could be transformed if needed. This now seems unnecessary, as any Butler instance should get the same dataIds for the same data from astro_metadata_translator.
I'm changing this to instrument, as it still feels like we need some additional information in the provenance, and I think that's provided by that.

python/lsst/ip/isr/calibType.py Show resolved Hide resolved
python/lsst/ip/isr/calibType.py Outdated Show resolved Hide resolved
IsrCalib should be the base class used by calibration data.
IsrProvenance supplies a concrete example subclass that will allow
calibrations to store their inputs in a way that can be used to
reconstruct the calibration.
Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

Some minor comments. Code looks much simpler without afw. Did it make any difference to I/O performance?

python/lsst/ip/isr/calibType.py Outdated Show resolved Hide resolved
associated metadata.

"""
if format == 'yaml' or (format == 'auto' and filename.lower().endswith((".yaml", ".YAML"))):
Copy link
Member

Choose a reason for hiding this comment

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

I think this means that I can pass in a file name of "hello.ecsv" and say "format=yaml" and end up with a yaml file that has a .ecsv extension. In Defects I force the extension to be correct and I think you have to do that here -- it's why the returned filename doesn't have to match the supplied filename.

calib.updateMetadata(setDate=False, **dictionary['metadata'])
calib._detectorName = dictionary['detectorName']
calib._detectorSerial = dictionary['detectorSerial']
calib.instrument = dictionary['metadata']['INSTRUME']
Copy link
Member

Choose a reason for hiding this comment

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

In fromTable doesn't an "instrument" field get filled in to the dict so I'm not sure why we need to look in the metadata here.

@czwa
Copy link
Contributor Author

czwa commented Apr 28, 2020

Running a timing test shows that the new astropy code is significantly faster than afwTable in 100 iterations with timeit:
afwTable (ip_isr@cf0b964): read/write: 21.2s; write only: 10.1s
astropy(ip_isr@tickets/DM-24537): r/w: 8.3s; w: 5.7s

These types are not used in C++, so it's faster and less confusing to
use astropy tables instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants