-
Notifications
You must be signed in to change notification settings - Fork 2
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
Be/create ifu wkf #89
Conversation
…o be/create_ifu_wkf
Nicely done! I can rebase The question is, should I? |
When I tried to run
|
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.
Nice! Thanks @eiseleb47 .
I'm not sure I'm still qualified to review this properly :-). But looks fine too me.
I'm not sure whether these 'pro' classes like sci_reduce_ifu_class
now allow us to use an existing file with PRO.CATG
of IFU_SCI_REDUCED
in the pipeline. At least I'm assuming that is the intent. But we can iterate on this later.
You can resolve @sesquideus remark about could not resolve targets
by adding a 'science' meta target. See #88 on how to do so.
Perhaps you could import everything in metis_wkf.py
?
Thank you for the quick feedback from both of you! The missing I have also added an import statement to the |
In #90, I've branched from this branch and let github action run the full EDPS workflow, so also the IFU one, see this commit: ea6ac56 That failed because this branch is too far behind main, so I merged main into #90 , and now it does run all the IFU recipes in the github action, but most of them fail. That would mean that the EDPS IFU workflow probably only works on your machine, and not on those of the rest of us. Could you please either merge #90 into this branch, or otherwise merge main into this branch and manually make the change of ea6ac56 , and then make sure the workflow actually runs on github? FWIW, here is the output of the test of #90 : https://github.com/AstarVienna/METIS_Pipeline/actions/runs/13102135125/job/36551697376
So it tries to run all 11 IFU recipes, but only 4 succeed. |
I managed to rebase |
I have merged #90 into the branch. The issue was in the rsrf task. It wasn't including a I've added the class, data source and associated input. Additionally I forgot adding the sky raws to the sci and std tasks. Added those as well. Lastly it still fails on two tasks, namely the calibrate and post process task. This is since the telluric recipe right now only produces a |
It could very well be that either the recipe and/or the DRLD is wrong. Comparing with similar recipes and with the templates ( https://metis.strw.leidenuniv.nl/wiki/doku.php?id=operations:metis_templates ), I'm not sure what is right. The most general thing is to keep the
We decided to change how the telluriccorrection is approached(AstarVienna/METIS_DRLD#310), so I suggest to just do whatever is necessary to make this work. E.g. either add the But I do think we should only merge if the tests pass. (Or more specifically, if the workflow runs on the test data, which should be captured by the tests.) |
Small note, looking at the graph of the repository, it seems you indeed rebased The above doesn't matter if Also, if we want to add more EDPS tests, I suggest to directly do that in the ESO-way; see the HAWK-I example in https://github.com/AstarVienna/hawki-edps-tests |
Sorry, yes, I failed to mention that |
Hi all - just a comment on the RSRF reipe and WCU_OFF_RAW frames. I had some discussions about this recipe during the METIS CM and the DRLD is out of date on a number of aspects that still needs TBC. The METIS Calibration Plan specifically calls for the WCU_OFF frames, for instance. For now I agree we can make these inputs optional in the skeleton recipe, but this will probably change going forward. |
I've tried merging main into the branch to get the updated Telluric Recipe from #94 . However it seems to be failing locally with this error: It passed the test in #94 because the The test it ran now seems to also have passed the telluric recipe. I'm not quite sure why since I'm using a clean installation of the branch. Could you perhaps run it as well to see if it is an issue with something on my machine? |
I am not sure what the problem might be. When I run this against |
That's weird. I'm using that branch as well. Do you mean the
Error:
|
By itself it should be fine, the warning should be removed anyway since this whole recipe is about IFU and detecting the detector does not make much sense. Could you please run something like this and show me?
|
The FileIOError might mean that you didn't set your paths right? For me,
The
|
@sesquideus The output looks like this for me:
When trying to execute it via the python command line I got this result when I tried to select the sof file:
@hugobuddel perhaps it does have to do with the paths. Currently I have them set up like this:
However they do seem to be the same as in the |
@eiseleb47 Ah yes, I see now. You can get the full output with |
There are some issues with the sofFiles, $SOF_DATA and where the intermediate files end up. I generated the release sof files with the assumption that all the files would be in the same location, $SOF_DATA, but if you run a recipe at the pyesorex level, it writes the output to the directory in which you run the command, which means it can't find files generated in an earlier step. I think we could solve that by calling pyesorex --output-dir $SOF_DATA which would keep all the data in one place. |
I set it directly in my shell rc:
though I don't know if this solves everything -- it does locally, in the sense that eventually all tests run if repeated long enough. |
On the telecon this morning we determined that the missing
@janusbrink offered to fix it |
The
But of course it only works if the recipes actually produce the data that they should, see comment above. |
I set the variable for I've held of on merging the rest of main into this branch to make sure that it would work since the tests for the newest commit don't pass as of now. Additionally, right now there is no implementation of the optional input for the callibration task like it is shown in the IFU flowchart in the DRLD. Thus I am not sure if it takes the IFU Telluric outputs from the SCI or STD task. However I'd say, since it is still a skeleton, this would be something for the next realease. |
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.
Everything does seem to work:
00h00m05s {'COMPLETED': 23}
23 jobs processed in 00h00m05s
You can find reduced data, logs and quality control plots in the following directories:
=======================================================================================
/tmp/EDPS_data/METIS/metis_ifu_lingain/88314499-06b6-49f0-9b2b-0eb5e6c91d7e
/tmp/EDPS_data/METIS/metis_ifu_dark/3877b49b-4999-49b6-b202-8213f60f1b64
/tmp/EDPS_data/METIS/metis_ifu_distortion/6a09a14e-cf19-4ec3-b336-3293b3836bcf
/tmp/EDPS_data/METIS/metis_ifu_wavecal/a7b44378-1b2c-4468-9713-3b975ef181eb
/tmp/EDPS_data/METIS/metis_ifu_rsrf/85915550-553e-4dba-90ea-202af2220360
/tmp/EDPS_data/METIS/metis_ifu_std_reduce/f0891cc5-a446-4c9f-bc8d-0fae837c91d2
/tmp/EDPS_data/METIS/metis_ifu_std_telluric/aee6b475-c76d-4b43-8bf4-394b4927e7f9
/tmp/EDPS_data/METIS/metis_ifu_sci_reduce/d79704b2-5dee-415f-92b6-8b178e69e1d5
/tmp/EDPS_data/METIS/metis_ifu_sci_telluric/31c02d8e-c73b-4982-aa0d-028e5fd12ee5
/tmp/EDPS_data/METIS/metis_ifu_calibrate/e0832b92-71c8-4664-ba2c-1354ea80cedd
/tmp/EDPS_data/METIS/metis_ifu_postprocess/70734119-45cf-4a66-bb2f-5dc0e20736e3
/tmp/EDPS_data/METIS/metis_det_dark/da0e776a-4377-4190-8852-1486571cb3fe
/tmp/EDPS_data/METIS/metis_det_detlin/0c60d122-c1c6-44a0-b56d-80c0ff7edffa
/tmp/EDPS_data/METIS/metis_lm_img_cal_distortion/607d3f07-cae3-42cf-a99d-68bf081fad86
/tmp/EDPS_data/METIS/metis_lm_img_flat/5a0ee546-d007-4baa-82cc-2e18bd8500c7
/tmp/EDPS_data/METIS/metis_lm_img_basic_reduce_sky/f71b8904-5490-4f24-8c77-c1fd5d9b858e
/tmp/EDPS_data/METIS/metis_lm_img_basic_reduce_std/d3332c90-1a24-4bc9-b9b7-372f695fee93
/tmp/EDPS_data/METIS/metis_lm_img_background_std/723f1908-73f3-4c4a-99bc-aa65b5eb43ca
/tmp/EDPS_data/METIS/metis_lm_img_standard_flux/d95302a4-9e86-4245-803f-f61da3a5c79c
/tmp/EDPS_data/METIS/metis_lm_img_basic_reduce_sci/69036d74-af33-46e3-9d20-a765e9152ac6
/tmp/EDPS_data/METIS/metis_lm_img_background_sci/3dccfde5-11fd-4119-aa07-a320a12d4f29
/tmp/EDPS_data/METIS/metis_lm_img_calib/c8df9377-994b-490d-b98d-9f1dce1ef6af
/tmp/EDPS_data/METIS/metis_lm_img_coadd/3f7b18ad-ba51-4121-9987-02e589ee4e49
Works for me too. |
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 was able to run everything and got COMPLETED.
I'd like to merge the IFU workflow skeleton into main as it is working as of now up till the
calibrate_ifu_task
.I believe the issue stems from somewhere in in the metis_ifu_telluric recipe as the output of it creates an IFU_TELLURIC.fits with a
FLUXCAL_TAB
tag. Thus the calibrate task can't find aIFU_TELLURIC
frame and fails.However I've seen that there were mulitple changes done to the recipes on main, including the switch to tags. So I wanted to leave them alone on my branch for now and see if it works with the changes.