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

Quick (science) reduction for calibration frames #45

Merged
merged 4 commits into from
Mar 6, 2024
Merged

Conversation

ajmejia
Copy link
Contributor

@ajmejia ajmejia commented Dec 1, 2023

This PR enables the reduction of calibration frames like twilights and lamp exposures with the quick-reduction script. The outputs still be the same as with the science reductions.

@ajmejia ajmejia self-assigned this Dec 1, 2023
@ajmejia ajmejia added the enhancement New feature or request label Dec 1, 2023
@ajmejia ajmejia marked this pull request as ready for review December 1, 2023 18:19
@havok2063
Copy link
Collaborator

havok2063 commented Dec 1, 2023

I'm a little confused by this PR. If this is still a draft work-in-progress, it should be converted to a draft PR.

Why are we adding in calibration frame reductions into this code? Calibration frame reductions are not a priority for LVM, at least not right now. We already have code to reduce calibration frames that I wrote that works within the main DRP code. See https://github.com/sdss/lvmdrp/blob/master/python/lvmdrp/functions/run_drp.py#L1536 and its use at

reduce_calib_frame(row)
.
This can be easily turned on with a flag. If we want to reduce the frames beyond detrending I think it's easier to update the code here, and keep the reductions separate from the science.

It looks like the quick reduction script will now reduce all frames by default, which I don't think is what we want, and since there are no tests for the pipeline, could introduce unknown issues, now that we have a running pipeline at Utah.

Why are we running the combine_channel and combine_spectrographs code on calibration frames? I'm not sure we want the same outputs for the calibration frames as the science frames. As written, this will write lvmCFrame files for these exposures. We should not be conflating the names of data products for their purpose. We have planned data products for lvmArc and lvmFlat files, which should be used here. The pipeline should reduce the cals exactly up to where these new files should be written out and then exit the pipeline.

We may want to shift focus on creating the lvmFrame, lvmSFrame, lvmFFrame, lvmRSS, and drpall data products, for the science reductions.

@ndrory
Copy link
Contributor

ndrory commented Dec 1, 2023 via email

@havok2063
Copy link
Collaborator

I understand that. I'm not saying cals shouldn't be reduced. I think my points are still valid ones. If we actually need calibration frames to be camspec-combined, then at the very least, bias, darks, arc, and flats, should be given different names so people can easily distinguish them from science data.

An additional point to keep in mind is, with the number of cals we have each night, at ~10-min per exposure, maybe less without sky and fluxcal processing, this will increase the reduction times at Utah, even with parallelization.

@ajmejia ajmejia merged commit a4ab903 into master Mar 6, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants