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-38944: Move photodiode computations from linearity to ptc code. #204
Conversation
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.
One minor question.
tests/test_linearity.py
Outdated
@@ -85,6 +87,9 @@ def _create_ptc(self, amp_names, exp_times, means, ccobcurr=None): | |||
""" | |||
exp_id_pairs = np.arange(len(exp_times)*2).reshape((len(exp_times), 2)).tolist() | |||
|
|||
if photo_charges is None: | |||
photo_charges = np.zeros(len(exp_times)) + np.nan |
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.
Is this more efficient than np.full(len(exp_times), np.nan)
?
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.
Oddly enough, for small N the zeros is faster (but negligible time), for medium N full is faster, and for large N they're very close. I don't think it matters much, I can change to full if you like.
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.
(small ~10; medium ~10000; large ~1000000)
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.
I assumed they were both extremely optimized, I think I just find full
easier to read.
2cd9d1f
to
a57dddf
Compare
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 fine to me. One question about the bad photodiode entry in the linearity tests.
tests/test_linearity.py
Outdated
pd_calib.currentScale = -1.0 | ||
pd_calib.integrationMethod = "CHARGE_SUM" | ||
# Add one bad photodiode value, but don't put it at the very | ||
# end because that will mess up the spline node positions. |
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.
Is this a problem that we need to check for and deal with? I'm assuming it's not a trivial fix given this note.
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.
Ah, sorry this is not clear. The issue is simply that I want the spline positions in the test code ("truth") to match the spline positions in the data, which are computed from 0 to max. So I don't want to mess with max or else the tests won't be comparing the same numbers.
a57dddf
to
4283018
Compare
Masking out a value slightly increased the noise in the fit, leading to the need to relax tolerances slightly. These tolerances are somewhat arbitrary.
This moves the photodiode connections and integration code from the linearity code to the PTC code. This will use the photoCharges part of the PTC dataset which was previously unfilled.
This also updates the default PTC and linearity pipelines for LSSTCam as appropriate.