Move lampstat check #1822
Move lampstat check #1822
Conversation
There was a problem hiding this comment.
My opinion is that this lamp checking code has no place in image combination tasks. I consulted you about this because I think I saw that you added it, but I could be wrong. This image combine task should run on images that have been vetted to be good elsewhere. This should be done in calibrations. That reduces the number of inputs and simplifies the image combining code here.
| # Load raw image | ||
| rawImage = rawimage.RawImage(ifile, spectrograph, det) | ||
| # Process | ||
| rawImage_list.append(rawImage.process( | ||
| frame_par['process'], scattlight=scattlight, bias=bias, | ||
| bpm=bpm, dark=dark, flatimages=flatimages, slits=slits, mosaic=mosaic)) | ||
|
|
||
|
|
There was a problem hiding this comment.
My opinion is that this code has no place in image combination tasks. The task should run on images that have been vetted to be good elsewhere. This should be done in calibrations. That reduces the number of inputs and simplifies the code here.
There was a problem hiding this comment.
That's right - and I don't disagree. Indeed, there is a TODO item about this because this should really come from the fitstbl so we don't need to keep reading in files multiple times for just one reduction, but this is not currently possible. The lamp check needs to be something that's checked when images are being combined (it's the same as exposure times). It's inherently a check to do with frame combination. The reason I have moved the check is so that you don't need to add the fudge function into jwst_nirspec.
So, can you please confirm - Does this code change at least resolve the issue with you having to add a fudge function to jwst_nirspec?
As far as I see it, I think there are two possibilities:
- The
calibrations.pycode calls this function when building calibrations images, so it would be easiest to manage the lamp check code here. To ensure it's just the calibrations that execute the lamp check, we can add an argument to this function (e.g.check_lamps=False) and set it to True when the calibrations call the buildimage. - We create a separate function that is only in calibrations.py to check the lamps before we pass it in to
buildimage. This is what you are advocating, I think, but it comes with an overhead of re-reading multiple fits files over and over again, and also has some duplicate code.
In my opinion, option 1 is cleaner, and I'm happy to implement the changes to support that. How do you feel?
There was a problem hiding this comment.
Thanks @rcooke-ast, I see your point. I don't think we need to reread the files, only the headers. I would prefer to check neither exposure times nor lamps in a combine function as it reduces the dependencies and simplifies the code. I'm fine with leaving the exposure times in there for now (perhaps a todo). We don't do anything about this anyway, just throw warnings.
There was a problem hiding this comment.
@jhennawi - I have moved this to calibrations now, and should make it easier to add calibrations checks in the future. I think this is what you were wanting, correct? I've left the exposure time check where it is for now, as I feel that is somewhat more important to be there.
I think we should deal with this now, so that you don't need to add a fudge function into the jwst_nirspec code. I've refactored this a few times now, and I think I keep misinterpreting what you would like to see in the code. If there's something specific that you would like changed, could you please comment on the PR? Thanks!
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## combine_refactor #1822 +/- ##
=================================================
Coverage 38.49% 38.50%
=================================================
Files 207 207
Lines 48157 48171 +14
=================================================
+ Hits 18538 18546 +8
- Misses 29619 29625 +6 ☔ View full report in Codecov by Sentry. |
| @@ -209,6 +210,44 @@ def __init__(self, fitstbl, par, spectrograph, caldir, qadir=None, | |||
| self.success = False | |||
| self.failed_step = None | |||
|
|
|||
| def check_calibrations(self, file_list, check_lamps=True): | |||
There was a problem hiding this comment.
Can you make it more clear what this routine does? It does not return anything and it does not appear to crash out if the calibrations are not okay. It appears to only print warnings to the screen. Can you please make that clear in the docs.
pypeit/images/buildimage.py
Outdated
| @@ -254,14 +254,14 @@ def buildimage_fromlist(spectrograph, det, frame_par, file_list, bias=None, bpm= | |||
|
|
|||
| rawImage_list = [] | |||
| # Loop on the files | |||
| for ifile in file_list: | |||
| for ii, ifile in enumerate(file_list): | |||
There was a problem hiding this comment.
Thanks @rcooke-ast. Only a very minor comment. Sorry for the delay as I was traveling.
There was a problem hiding this comment.
I just made the quick changes I requested myself. Thanks @rcooke-ast.
|
Merging! |
|
You just beat me to it - I was in the process of editing - thanks!! |
Moving the lamp check to outside the
CombineImagetask. Unit tests are good on my machine.