-
Notifications
You must be signed in to change notification settings - Fork 41
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
Make Readnoise Monitor less memory-intensive #922
Make Readnoise Monitor less memory-intensive #922
Conversation
Hello @bsunnquist, Thank you for updating !
Comment last updated at 2022-04-13 16:46:16 UTC |
@bhilbert4 this is ready for review whenever. It should solve all of the ongoing readnoise monitor issues (assuming also the jwst pipeline is updated to 1.4.3) |
@bsunnquist thank you for the submission, I went ahead and had a few questions/comments. |
thanks @mfixstsci , did your review post? I don't see any questions, just that one comment so far |
@@ -542,16 +548,23 @@ def run(self): | |||
siaf = Siaf(self.instrument) | |||
possible_apertures = list(siaf.apertures) | |||
|
|||
# Get the minimum groups needed to calculate readnoise for this instrument. MIRI | |||
# removes the first five and last group before calculating the readnoise, so needs extra. | |||
if instrument == 'miri': |
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.
Were all minimum groups set to a higher value previously?
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.
yes, and it missed the cases where there were lots of ints but low groups, which should still be able to give a good cds readnoise measure. Though this comment reminded me of something, so I made another tweak to this logic in a new push.
@@ -181,5 +181,11 @@ def update_mean_bias_figures(self): | |||
if len(bias_vals) != 0: | |||
self.refs['mean_bias_xr_amp{}_{}'.format(amp, kind)].start = expstarts.min() - timedelta(days=3) | |||
self.refs['mean_bias_xr_amp{}_{}'.format(amp, kind)].end = expstarts.max() + timedelta(days=3) | |||
self.refs['mean_bias_yr_amp{}_{}'.format(amp, kind)].start = min(x for x in bias_vals if x is not None) - 20 | |||
self.refs['mean_bias_yr_amp{}_{}'.format(amp, kind)].end = max(x for x in bias_vals if x is not None) + 20 | |||
min_val, max_val = min(x for x in bias_vals if x is not None), max(x for x in bias_vals if x is not None) |
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.
Thanks for tackling the same issue with the bias monitor as well.
@bsunnquist I guess I didn't hit the big green button 😓 let me know if you see it now. |
thanks @mfixstsci I just made one more minor tweak to the minimum group/int logic, but let me know if you see any issues. This latest strategy will make sure images with lots of ints/low groups will still get processed, but also ensure the total number of these cds frames is still large enough to give meaningful measurements |
and one more tweak, to push back the lookback time a little more to make sure it picks up the earliest dark data |
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 to me!
This PR does the following
The first point is the main one - it addresses the memory issues noted in #905 , and (after the jwst pipeline is switched to jwst1.4.3) should also solve #900.