sdss / lvmdrp

Local Volume Mapper (LVM) Data Reduction Pipeline
BSD 3-Clause "New" or "Revised" License
2 stars 0 forks source link

Quick (science) reduction for calibration frames #45

Closed ajmejia closed 6 months ago

ajmejia commented 9 months ago

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.

havok2063 commented 9 months ago

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 https://github.com/sdss/lvmdrp/blob/448331b959767fd5edeefba24cb91d9c2eb1d605/python/lvmdrp/functions/run_drp.py#L1513.
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 commented 9 months ago

Brian,

The instrument team has requested full reductions of calibration frames to monitor instrument changes long term.

N.

On Dec 1, 2023, at 3:49 PM, Brian Cherinka @.***> wrote:

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 https://github.com/sdss/lvmdrp/blob/448331b959767fd5edeefba24cb91d9c2eb1d605/python/lvmdrp/functions/run_drp.py#L1513. 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, and drpall data products, for the science reductions.

— Reply to this email directly, view it on GitHubhttps://github.com/sdss/lvmdrp/pull/45#issuecomment-1836824119, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AJXJJ7KLJ45EZJBLUZJRNXLYHJGF3AVCNFSM6AAAAABADEA6GOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZWHAZDIMJRHE. You are receiving this because you are subscribed to this thread.Message ID: @.***>

-- Niv Drory — McDonald Observatory / Dept. of Astronomy The University of Texas at Austin Tel: +1 512 471 6197 http://www.as.utexas.edu/~drory

havok2063 commented 9 months ago

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.