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-44025: Update ptc turnoff computation code and improve tests with real data. #240
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.
Just a few clarifications.
dtype=float, | ||
doc="If there are any outliers in the initial fit above " | ||
"maxSignalInitialPtcOutlierFit, then no points that have this delta " | ||
"from the previous ``good`` point are allowed. If " |
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.
Can you clarify that this delta is in terms of the rawMeans
vector (or finalMeans
). The units being to first order implies this (so that it's not based on the variance).
@@ -1236,8 +1276,10 @@ def errFunc(p, x, y): | |||
|
|||
count += 1 |
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.
Do we not warn if we've reached the iteration threshold and are still finding outliers? I'm guessing we converge very quickly, so it's possible 2 is always sufficient, but I wanted to check if this was an oversight.
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.
We never have, but I can add a warning.
tests/test_ptc.py
Outdated
solvedDataset = solveTask.fitMeasurementsToModel(dataset) | ||
|
||
# Check that the ptcTurnoff is what is expected. | ||
self.assertFloatsAlmostEqual(solvedDataset.ptcTurnoff[ampName], ptcTurnoff) |
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.
Can you add msg
arguments to all of these asserts with the dense
and mode
parameters? That should help identify problems when these tests fail in the future.
if dense and mode == "normal": | ||
# Taken from dense run 13591, detector 94, amplifier C02 | ||
ptcTurnoff = 92239.4794 | ||
rawMeans = np.array([ |
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 have a slight preference for storing these as external PTC dataset files in tests/data
, but it's probably not worth the effort on this ticket.
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.
Since we didn't have a test data directory I was too lazy to make one ...
This also adds a computation of the "sampling error" on the ptc turnoff.