PIPE2D-1770: Isolate ApplyFluxCalTask from FitFluxCalTask #385
Conversation
SogoMineo
force-pushed
the
tickets/PIPE2D-1770
branch
from
3c2b45b to
e966fa0
Compare
PaulPrice
left a comment
PaulPrice
left a comment
There was a problem hiding this comment.
I like that we're splitting up the fitFluxCal.py file: it was getting very large.
Good work!
| pfsCalibrated = OutputConnection( | ||
| name="pfsCalibrated", | ||
| doc="Flux-calibrated object spectrum", | ||
| storageClass="PfsCalibratedSpectra", # deprecated in favor of PfsCalibrated |
bin.src/ingestPredefinedFluxCal.py
Outdated
| ingestPredefinedFluxCal(args.repo, args.instrument, args.run, args.path, args.transfer) | ||
|
|
||
|
|
||
| def ingestPredefinedFluxCal( |
There was a problem hiding this comment.
Could you please put this function in python/pfs/drp/stella/gen3.py?
bin.src/ingestPredefinedFluxCal.py
Outdated
|
|
||
|
|
||
| def main() -> None: | ||
| """Ingest a predefined fluxCal file into a gen3 datastore.""" |
There was a problem hiding this comment.
How are the predefined fluxCal files generated? The user writes a PfsFluxCalib however they want?
There was a problem hiding this comment.
There is not a concrete plan yet. For tests, I just re-ingested a usual fluxCal as fluxCal_predefined. But in reality, we need to take average of many fluxCals in some way.
| @@ -0,0 +1,6 @@ | |||
| description: Calibrate exposure with predefined fluxCal | |||
There was a problem hiding this comment.
Maybe add a comment that an empty quantum graph may be due to not being able to find the fluxCal.
There was a problem hiding this comment.
I will change the description to:
Calibrate exposure with a single predefined fluxCal
applied to every exposure.
This pipeline requires a "predefined fluxCal",
which is of the same file type as,
but is distinguished from, a usual fluxCal.
It is a prerequisite ingested with ingestPredefinedFluxCal.py.
e966fa0 to
da66a51
Compare
SogoMineo
force-pushed
the
tickets/PIPE2D-1770
branch
from
da66a51 to
b849d51
Compare
| parser.add_argument( | ||
| "fluxCal", | ||
| help=""" | ||
| Path to a predefined fluxCal |
There was a problem hiding this comment.
Where does this file come from? If it's coming from the results for a different visit, I think it would be better to specify a collection and visit, and have the code retrieve the file from the butler.
There was a problem hiding this comment.
I was told that the fluxCal file will be made by taking average, in some way, of an arbitorarily chosen set of fluxCal files. I thought the program to do it would be a quick-and-dirty jupyter notebook, and therefore I had fluxCal specified by a path. If we write a pipeline to take the average, we will have to define a new dataset type fluxCal_predefined (or fluxCal_average?) which does not depend on visit dimension.
bin.src/ingestPredefinedFluxCal.py
Outdated
| args = parser.parse_args() | ||
|
|
||
| input_collections = [x.strip() for x in args.input.split(",")] | ||
| input_collections = [x for x in input_collections if x] |
There was a problem hiding this comment.
Is it necessary to process the args.input? I think the butler does all this for you, doesn't it?
There was a problem hiding this comment.
According to my experiment, comma-separated collection names in a single string was not recognized by Butler.
>>> butler=Butler.from_config(config=".", collections="test,test2,test3")
>>> butler.query_datasets("fluxCal", find_first=False)
lsst.daf.butler._exceptions.MissingCollectionError: No collection with name 'test,test2,test3' found.
bin.src/ingestPredefinedFluxCal.py
Outdated
| if not os.path.isfile(args.fluxCal): | ||
| raise RuntimeError(f"'{args.fluxCal}' is not found or is not a file.") | ||
|
|
||
| butler = Butler.from_config( |
There was a problem hiding this comment.
Please put the below code in pfs.drp.stella.gen3. While the code is short and simple, bug fixes may become necessary at some point, and those are easier when the functional code lives under python/ instead of bin.src/.
There is a need for a user-provided `fluxCal` to be used as an argument of FitFluxCalTask. We isolate ApplyFluxCalTask from FitFluxCalTask for such a need.
PfsCalibratedSpectra (in drp_stella) has been deprecated in favor of PfsCalibrated (in datamodel).
SogoMineo
force-pushed
the
tickets/PIPE2D-1770
branch
from
b849d51 to
b8b0e5a
Compare
This program registers a single fluxCal many times with various visit numbers so that the pipeline will use it to calibrate all these visits.
Don't give up radial velocity estimation for all fibers even if the estimation fails for a few fibers. Catch the exception and continue.
SogoMineo
force-pushed
the
tickets/PIPE2D-1770
branch
from
b8b0e5a to
928b5d9
Compare
There is a need for a user-provided
fluxCalto be used as anargument of FitFluxCalTask. We isolate ApplyFluxCalTask from
FitFluxCalTask for such a need.