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-38777: LinearizeSpline linearity corrections do not anchor the spline at zero flux #258

Merged
merged 2 commits into from Apr 26, 2023

Conversation

czwa
Copy link
Contributor

@czwa czwa commented Apr 24, 2023

Fix spline linearity-induced amplifier offsets.

@czwa czwa requested a review from taranu April 24, 2023 14:06
@@ -753,6 +756,14 @@ def __call__(self, image, **kwargs):
"""
splineCoeff = kwargs['coeffs']
centers, values = np.split(splineCoeff, 2)
if centers[0] != 0.0 or values[0] != 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this deliberately or rather than a nested if as in cp_pipe?

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 thought it was correct as there are two issues to correct, and then discovered that giving the interpolation code centers like [0.0, 0.0, 1.0, 2.0, 3.0, 4.0, 5.0] causes an error even if the values for the two 0.0 points is identical. I've split the two issues into separate checks, which should prevent that interpolation error.

@@ -125,8 +125,8 @@ def setUp(self):
[[1e-5, 1e-7], [1.1e-6, 1e-7], [2.1e-6, 1e-7]]], dtype=float)

# Spline coefficients: should match a 1e-6 Squared solution
self.splineCoeffs = np.array([-100, 0.0, 1000, 2000, 3000, 4000, 5000,
0.0, 0.0, 1.0, 4.0, 9.0, 16.0, 25.0])
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, presumably this would have generated at least slightly negative values between -100 and 0 before, no? Did that cause any issues? But either way, starting it at zero definitely seems to make more sense.

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 think it was just poor thinking on my part when I wrote the test. If that does match the 1e-6 squared solution, then that negative point gains flux from a correction entirely derived from the positive data, which doesn't make any sense at all.

if self._detectorSerial != detector.getSerial():
raise RuntimeError("Detector serial numbers don't match: %s != %s" %
(self._detectorSerial, detector.getSerial()))
# DM-38778: This check fails on LATISS due to an error in the
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider prefacing this and below note with TODO: Uncomment when DM-38778 is complete..., if you find TODO tags helpful.

@czwa czwa merged commit 4b25345 into main Apr 26, 2023
1 check passed
@czwa czwa deleted the tickets/DM-38777 branch April 26, 2023 22: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
Development

Successfully merging this pull request may close these issues.

None yet

2 participants