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-21739: Enable and test fringe/strayLight in gen3 #116
Conversation
ce35358
to
1c70739
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.
Approve conditioned on addressing the small comments
python/lsst/ip/isr/isrTask.py
Outdated
# straylightRef = inputs['strayLightData'] | ||
# diskUri = straylightRef.getUri() | ||
# inputs['strayLightData'] = diskUri | ||
print("Recieved straylightData: ", inputs['strayLightData']) |
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.
Probably remove this debug print and put back in the comments?
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'm rewriting this, as butler now handles StrayLightData objects. This mitigates the need for the getUri, as inputs['strayLightData'] now contains the object itself.
python/lsst/ip/isr/fringe.py
Outdated
fringe = assembler.assembleCcd(fringe) | ||
fringeExp = assembler.assembleCcd(fringeExp) | ||
|
||
seed = self.config.stats.rngSeedOffset + 12345678 |
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.
why this magic number, if no reason add a comment. If there is a reason, add a comment
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.
Updated to try to use the exposureId as in gen2 instead of a magic number.
python/lsst/ip/isr/isrTask.py
Outdated
# straylightRef = inputs['strayLightData'] | ||
# diskUri = straylightRef.getUri() | ||
# inputs['strayLightData'] = diskUri | ||
print("Recieved straylightData: ", inputs['strayLightData']) |
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 think you dont want to print here, but add it to inputs?
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.
As above, rewritten. inputs['strayLightData'] should get what it says from the butler, and if not, rely on the straylight task to raise if it can't handle things.
tests/test_fringes.py
Outdated
self.assertFloatsAlmostEqual(medianAfter, 3002.233, atol=1e-4) | ||
self.assertFloatsAlmostEqual(stdAfter, 3549.9375, atol=1e-4) | ||
self.assertFloatsAlmostEqual(medianAfter, 3001.312, atol=1e-4) | ||
self.assertFloatsAlmostEqual(stdAfter, 3549.949, atol=1e-4) |
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.
What did these values change?
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.
These values are related to how the fringe seed value is set. The tolerance likely should be smaller to more accurately represent the error in this measurement due to changes in seed, but I'm not going to worry about it on this ticket.
These seem to differ from those made by stand-alone repos.
25d3be0
to
c9cfb45
Compare
No description provided.