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-36597: Add LATISS filter transmission data #12
Conversation
czwa
commented
Nov 8, 2022
- Add LATISS filter transmission curves.
- Update existing LSSTCam and TS8 sensor transmission curves to new IntermediateTransmissionCurve format.
* LATISS: * This involved migrating back to the chip-based path. * Updated the CALIBCLS to point to the subclasses. * TS8/LSSTCam: * Moved to "transmission_sensor" paths. * Updated metadata. Of the following, the first updates to the preferred dataset type, and adds a CALIBCLS. The second was needed to fix that CALIBCLS to fix ingest. * find . -type f -iname '*ecsv' -exec sed -i.bak 's/QE/transmission_sensor/; s/qe_curve/transmission_sensor/; s/# - {PICKLEFILE/# - {CALIBCLS: lsst.ip.isr.IntermediateTransmissionCurve}\n# - {PICKLEFILE/' {} + * find . -type f -iname '*ecsv' -exec sed -i.bak2 's/IntermediateTransmissionCurve/IntermediateSensorTransmissionCurve/' {} +
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, two minor comments.
No negative values were clipped in any band, although they were likely | ||
caused by random noise during measurement. For consistency with other |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whether noise or light leaks, I wonder if we should clip them. Seems like having negative values could be problematic and/or confusing, and while it might not be "right", I feel like it can't be the wrong call, given they're literally unphysical values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The negative values are almost certainly sampling noise in the measurements. I did think about clamping everything smaller than 5e-5 to zero, as that would also eliminate the values at that scale that pop-up at the ends of the transmission. I didn't want to do much value processing, to keep the on-disk values as close to the raw measurements as possible, letting the user decide what to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree, it's probably best to let code using the values deal with these, it's a single line to clip them all, so I think on balance this is indeed the best course here.
# --- | ||
# datatype: | ||
# - {name: wavelength, unit: nm, datatype: float32} | ||
# - {name: throughput, unit: '', datatype: float32} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Files below use -
for the dimensionless value, but this one uses ''
here and on L22. Does that matter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
astropy doesn't like anything for dimensionless units (as far as I can tell), so I think these are equivalent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine, just thought they should all agree. I'm sure it doesn't actually matter though.