-
Notifications
You must be signed in to change notification settings - Fork 0
DM-50019: Add initial LSSTCam prompt processing pipelines #289
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
Conversation
647f991
to
9d8fccc
Compare
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'm a bit worried about the forward-compatibility of these changes. On the one hand, we won't be able to run single-frame processing (let alone the -noForced
or -noAlerts
variants, which if we need, we'll need in a hurry) without a new release. On the other hand, redefining Isr.yaml in a future release may cause confusion (for us, not just users!) about which one we're running.
I imagine there are complications with implementing the full set of pipelines we had for LATISS/ComCam?
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.
This looks more like an Isr-cal
than an Isr
... is this something we can run on early commissioning data?
I'm worried that naming this Isr will lead to some complicated cross-talk where we have to change our pipeline config depending on which PP release we're using.
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 set this config thinking it's for the data we can test with right now, and will change it completely once we have on-sky data. Causing confusion is a very good point. Isr-cal
is a great idea. I'm renaming this to Isr-cal.yaml
, and adding a standard Isr.yaml
which we can't test yet but should become right soon.
9d8fccc
to
d506b1c
Compare
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.
Looks good. I guess we'll write up the incomplete set of pipelines as a known issue?
I somewhat feel like we don't need to and can wait until there are real issues to file. There pipelines are valid pipelines (with all the carefully chosen defaults), just not tested with actual data yet. When we have actual data, things might just work. If not, we can file more specific issues. |
I'm not sure I follow. How can a pipeline (say, In any case, I'm fine with leaving that for later, I just think we should be clear up-front that those processing modes aren't supported in the next release. |
Oh, that! I misunderstood and thanks a lot for explaining. I'll add them. |
d506b1c
to
3600787
Compare
Only Isr-cal can be tested with the data on hand right now. But a standard set of yaml is added and they should be useful soon.
3600787
to
045dfc6
Compare
No description provided.