-
Notifications
You must be signed in to change notification settings - Fork 0
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
Updates DRP for runs at Utah #41
Conversation
@ajmejia this is ready to be merged. |
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 had just a few minor comments. We can actually merge this branch as it is.
One more general comment is that I'm aiming at keeping the reduction of science frames and calibration frames as separated as possible. Right now the reduction for bias, dark, pixelflat, fiberflat and arcs is going through preprocessing and detrending, and that's fine. But after that what we do to the frames may differ a lot depending on the type of frame and calibration sequence that we have.
from lvmdrp.functions.imageMethod import preproc_raw_frame | ||
from lvmdrp.functions.skyMethod import configureSkyModel_drp | ||
from lvmdrp.utils.metadata import get_frames_metadata, get_master_metadata | ||
|
||
from lvmdrp.functions.run_quickdrp import quick_reduction | ||
from lvmdrp.functions.run_quickdrp import quick_science_reduction |
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 this should be renamed to science_reduction
throughout, just to avoid confusions. It's not running a quick pipeline anymore.
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 can certainly rename this, but if it's no longer the case of being a quick pipeline, then we should reorganize the modules.
But this isn't doing optimal extraction, right? Nor Amy's full sky subtraction. Or a complete flux calibration? Presumably we will retain both quick and full modes once those are in place?
@@ -222,6 +219,8 @@ def quick_reduction(expnum: int, use_fiducial_master: bool, skip_sky_subtraction | |||
mlsf_path = path.full("lvm_master", drpver=drpver, kind=f"mlsf_{lamps}", **masters["lsf"].to_dict()) | |||
mflat_path = path.full("lvm_master", drpver=drpver, kind="mfiberflat", **masters["fiberflat"].to_dict()) | |||
|
|||
log.info(f'--- Starting science reduction of raw frame: {rsci_path}') |
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 would move this line to inside of the preprocessing routine. In general I'm trying to keep stuff like "doing X to file path/to/file" and "writing Xed output to path/to/file" inside the routine that does X. It keeps things a bit more tight and self-contained.
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'd like to have a general log line that indicates the start of a single file reduction, not any particular step.
path = root / f'lvm-drp-{tileid}-{mjd}.{status}' | ||
return path.exists() | ||
|
||
|
||
def update_error_file(tileid: int, mjd: int, expnum: int, error: str, | ||
reset: bool = False): | ||
"""_summary_ |
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.
The function is easy to read, but you may want to fill in this placeholder docstring.
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.
good catch
I think Nick in particular asked for extractions of nightly arcs and flats, so that the state of the instrument can be monitored by looking into longer-term trends. So reducing these frames at least through extraction could be helpful, and we should think about how to do it.
N.
On Nov 28, 2023, at 2:11 PM, Alfredo Mejia-Narvaez ***@***.***> wrote:
@ajmejia approved this pull request.
I had just a few minor comments. We can actually merge this branch as it is.
One more general comment is that I'm aiming at keeping the reduction of science frames and calibration frames as separated as possible. Right now the reduction for bias, dark, pixelflat, fiberflat and arcs is going through preprocessing and detrending, and that's fine. But after that what we do to the frames may differ a lot depending on the type of frame and calibration sequence that we have.
________________________________
In bin/drp<#41 (comment)>:
from lvmdrp.functions.imageMethod import preproc_raw_frame
from lvmdrp.functions.skyMethod import configureSkyModel_drp
from lvmdrp.utils.metadata import get_frames_metadata, get_master_metadata
-from lvmdrp.functions.run_quickdrp import quick_reduction
+from lvmdrp.functions.run_quickdrp import quick_science_reduction
I think this should be renamed to science_reduction throughout, just to avoid confusions. It's not running a quick pipeline anymore.
________________________________
In python/lvmdrp/functions/run_quickdrp.py<#41 (comment)>:
@@ -222,6 +219,8 @@ def quick_reduction(expnum: int, use_fiducial_master: bool, skip_sky_subtraction
mlsf_path = path.full("lvm_master", drpver=drpver, kind=f"mlsf_{lamps}", **masters["lsf"].to_dict())
mflat_path = path.full("lvm_master", drpver=drpver, kind="mfiberflat", **masters["fiberflat"].to_dict())
+ log.info(f'--- Starting science reduction of raw frame: {rsci_path}')
I would move this line to inside of the preprocessing routine. In general I'm trying to keep stuff like "doing X to file path/to/file" and "writing Xed output to path/to/file" inside the routine that does X. It keeps things a bit more tight and self-contained.
________________________________
In python/lvmdrp/functions/run_drp.py<#41 (comment)>:
path = root / f'lvm-drp-{tileid}-{mjd}.{status}'
return path.exists()
+def update_error_file(tileid: int, mjd: int, expnum: int, error: str,
+ reset: bool = False):
+ """_summary_
The function is easy to read, but you may want to fill in this placeholder docstring.
—
Reply to this email directly, view it on GitHub<#41 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AJXJJ7IVLTJODEDJ7UO25GDYGZANLAVCNFSM6AAAAAA7GNL3XGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONJTGY2TKOBXGQ>.
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
|
This PR updates the DRP so it can more easily run at Utah. It reorganizes the
quick_reduction
function so it can be called directly, asquick_science_reduction
. The original CLIquick-reduction
is retained so users can continue to use it. It deprecates the oldrun_drp
code and replaces it with a newrun_drp
that uses thequick_science_reduction
function. Italso adds new functions for performing MJD checks against the daily data transfer, and writing status run files. Exposures without a
tile_id
get assigned a fake id of 1111, and output reductions are placed there.It also includes the option of reducing the individual calibration files up through the
detrending
stage. This is currently turned off by default.Manual test runs are at https://data.sdss5.org/sas/sdsswork/lvm/spectro/redux/master/ . I'm testing out a cron job at the moment to run a daily check every hour between 12-17:00 Utah time. It currently takes ~10 minutes to reduce a single exposure, using the default settings of
quick_science_reduction
. MJD 60256 had ~28 science exposures and took ~5 hours in total. This is simply iterating over each exposure in a single process. If we want to parallelize this, additional work needs to done to use the Utah CHPC cluster with theslurm
package.