-
Notifications
You must be signed in to change notification settings - Fork 3
Karr/ifu hdi #167
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
base: main
Are you sure you want to change the base?
Karr/ifu hdi #167
Conversation
|
Thanks for cleaning up the workflow! Looks good |
| from . import metis_keywords as metis_kwd | ||
| from .metis_ifu_wkf import * | ||
|
|
||
| ifu_ravc_post_task = (task('ifu_ravc_post') |
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.
@eiseleb47 renamed all the tasks such that the names are consistent. That is, having each task name be the same as the recipe, optionally with a suffix. So it is immediately clear what recipe a task runs. Here this seems very much possible, e.g.:
| ifu_ravc_post_task = (task('ifu_ravc_post') | |
| metis_ifu_adi_racv_task = (task('metis_ifu_adi_racv') |
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 need to rework the workflow a bit; the RAW science input for the workflow will need a different classification rule than the standard lm_img sequence to select only the correct type of HCI images, so importing the whole lm_img workflow won't work.
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.
That would still only apply to the science part of the workflow right? That is, we can still keep the calibration part as-is.
Nevertheless, this was a comment about the naming of the tasks. For this particular task it should be feasible to give it a name that mimics the recipe, because this task is not duplicated from the LM workflow.
| lm_ravc_post_task1 = (task('lm_ravc_post1') | ||
| .with_recipe('metis_img_adi_cgrph') | ||
| .with_main_input(lm_img_calib_task1) | ||
| .with_main_input(lm_img_calib_task) |
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't comment on the task name, since that line is not edited, but here I suggest to have this as the first line of the task definition:
metis_lm_adi_racv_task = (task('metis_lm_adi_racv')
We already decided on the names of the recipes. Maybe those names are good or bad, but either way, it would be best to use similar names for both the tasks and the recipes. If the names are bad, we should change both.
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.
There is no band dependency in the recipe, so are you sure it is necessary to have a separate task for lm and for n?
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.
It might be good to keep LM and N separate for now, in case of differences in reduction. In any case, we'll be splitting the current giant recipes into more manageable chunks, and checking modularity of the routines will be done then.
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.
Yeah, we do need different tasks, that is true, even if they share the recipe. IIRC.
IFU RAVC workflow, plus tidying the LM RAVC.