Cy/issue 0033 #88

chyan26 · 2025-01-31T14:45:16Z

This branch has completed LM IMG pipeline skeleton. I noticed that the EDPS could not run with the current updated code in main branch. This branch contains several fixed and update, which may fix the problem. I suggest we merge this branch to main before getting too long.

sesquideus · 2025-01-31T21:28:12Z

I could fix the forgotten changes from main but apparently EDPS still does not like it.

hugobuddel

Supercool @chyan26 !! Great to see everything come together!

I've added some minor comments. Could you please look at them and reply with your thoughts?

I can't judge how all this fits into @sesquideus 's framework, so I'll let Martin do the 'real' review, I'm just an excited bystander

hugobuddel · 2025-01-31T21:02:11Z

metisp/pymetis/src/pymetis/recipes/img/metis_lm_img_background.py

+class MetisLmImgBackgroundImpl(RawImageProcessor):
+
+    class InputSet(RawImageProcessor.InputSet):
+        class RawInput(RawInput):
+            _tags = re.compile(r"LM_(?P<target>SCI|SKY|STD)_BASIC_REDUCED")


This change looks weird to me, because metis_lm_img_background does not have raw data as input. Maybe this is the way to do things, I don't know. Maybe @sesquideus can chime in?

This indeed should not be a RawInput, just a SinglePipelineInput or even better a dedicated class BasicReducedInput(SinglePipelineInput).

Agrees, I just picked up a class that works. We can changed to the current fashion.

hugobuddel · 2025-01-31T21:03:27Z

metisp/workflows/metis/metis_datasources.py

@@ -19,8 +19,8 @@


 # --- Data sources ---
-detlin_raw = (data_source()
-            .with_classification_rule(detlin_class)
+detlin_2rg_raw = (data_source()


Thank you! But why stop here and not also change lingain_task to lm_lingain_task etc?

Right, I should add lm to all related task.

metisp/pymetis/src/pymetis/inputs/multiple.py

hugobuddel · 2025-01-31T21:19:48Z

metisp/pymetis/src/pymetis/recipes/img/metis_lm_img_cal_distortion.py

+        class RawInput(RawInput):
+            _tags = re.compile(r"LM_(?P<target>WCU_OFF|DISTORTION)_RAW")


Are we sure this is how we want to model this input? I was expecting the WCU_OFF frames to be a separate calibration product, similar to how it is done in MetisIfuRsrfImpl. I could perhaps be convinced the other way though.

I am also not too sure about this part. This is based on DRLD. So, I would keep it this way for now.

The WCU_OFF_RAW are listed as calibration input in the DRLD, not as the main input. That's why I'm suggesting to have it as a calibration input in the workflow as well. It doesn't really matter from that perspective whether it is raw data or not. We did it that way in metis_ifu_rsrf too. We can also discuss whether we want to do things differently.

hugobuddel · 2025-01-31T21:20:51Z

metisp/workflows/metis/metis_lm_img_wkf.py

@@ -30,3 +38,56 @@
                    .with_meta_targets([SCIENCE])
                    .build())

+basic_reduction_sky = (task('metis_lm_img_basic_reduce_sky')


Please prefix all these tasks with lm, because otherwise they will conflict with the tasks from the other subinstruments.

hugobuddel · 2025-01-31T21:22:38Z

metisp/workflows/metis/metis_lm_img_wkf.py

+img_coadd = (task('metis_lm_img_coadd')
+             .with_recipe('metis_lm_img_sci_postprocess')


I strongly suggest keeping the task names as close to the recipe names as possible. Other names only add confusion.

hugobuddel · 2025-01-31T21:25:26Z

metisp/workflows/metis/metis_lm_img_wkf.py

+                    .with_recipe('metis_lm_img_background')
+                    .with_main_input(basic_reduction_std)
+                    .with_associated_input(basic_reduction_sky)
+                    .with_meta_targets([SCIENCE])


Should all these calibration tasks have SCIENCE as meta target? I would expect some other target like CALIB.

Correct, some of those should be CALIB.

hugobuddel · 2025-01-31T21:33:15Z

I could fix the forgotten changes from main but apparently EDPS still does not like it.

There are still some self.inputs += [ lists in the new recipes; I think those should now be sets, right?

sesquideus · 2025-01-31T21:37:43Z

There are still some self.inputs += [ lists in the new recipes; I think those should now be sets, right?

Yes, and that should fail TestInputSet::test_has_inputs_and_it_is_a_set. I hope it only occurs with recipes which do not have the corresponding test module yet.

Co-authored-by: Hugo Buddelmeijer <[email protected]>

hugobuddel · 2025-02-03T12:11:00Z

I don't understand @chyan26 . Above you explicitly agree to several comments, but then you merge it without addressing them. What is the plan?

hugobuddel · 2025-02-03T12:12:35Z

.gitignore

+# Skip __init__.py setting
+__init__.py


__init__.py should not be in .gitignore, since it should generally not be ignored. Could you please remove this again? I believe this was added only after I reviewed the PR so I couldn't remark on it before.

chyan26 linked an issue Jan 31, 2025 that may be closed by this pull request

REC : skel : metis_lm_img_background #33

Closed

12 tasks

chyan26 requested a review from sesquideus January 31, 2025 15:35

hugobuddel reviewed Jan 31, 2025

View reviewed changes

hugobuddel mentioned this pull request Feb 1, 2025

Be/create ifu wkf #89

Open

chyan26 and others added 21 commits February 3, 2025 10:57

Work for background recipe

e31831d

Allowing SKY target

e949c6b

Update README

b3d053d

Update program

853557b

EDPS works with background

3ec4870

Update the readme

febf578

Working with standard process

437a96c

Works with metis_lm_img_std_process

0508431

Adding to metis_recipes.py

65e56a3

Adding distorion recipe

dfc7eb6

Fixing typo

88f56a0

Works with metis_lm_img_cal_distortion

7b11a49

Working with metis_lm_img_calibrate

78a6416

Full functional version

b1c7a82

Update metis_recipes.py

1493aef

Update metisp/pymetis/src/pymetis/inputs/multiple.py

f78dd51

Co-authored-by: Hugo Buddelmeijer <[email protected]>

Update review suggestion

4470896

Fix bug

12c53aa

Fix bug for running EDPS

273eb08

Run edps test

0a2321a

No store __init__.py

bafce7b

chyan26 force-pushed the cy/issue-0033 branch from 33a1fe0 to bafce7b Compare February 3, 2025 10:00

chyan26 merged commit e8f98d1 into main Feb 3, 2025
2 checks passed

hugobuddel reviewed Feb 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cy/issue 0033 #88

Cy/issue 0033 #88

chyan26 commented Jan 31, 2025

sesquideus commented Jan 31, 2025

hugobuddel left a comment

hugobuddel Jan 31, 2025

sesquideus Jan 31, 2025

chyan26 Feb 1, 2025

hugobuddel Jan 31, 2025

chyan26 Feb 1, 2025

hugobuddel Jan 31, 2025

chyan26 Feb 1, 2025

hugobuddel Feb 3, 2025

hugobuddel Jan 31, 2025

hugobuddel Jan 31, 2025

hugobuddel Jan 31, 2025

chyan26 Feb 3, 2025

hugobuddel commented Jan 31, 2025

sesquideus commented Jan 31, 2025

hugobuddel commented Feb 3, 2025

hugobuddel Feb 3, 2025

		class RawInput(RawInput):
		_tags = re.compile(r"LM_(?P<target>WCU_OFF\|DISTORTION)_RAW")

		img_coadd = (task('metis_lm_img_coadd')
		.with_recipe('metis_lm_img_sci_postprocess')

Cy/issue 0033 #88

Cy/issue 0033 #88

Conversation

chyan26 commented Jan 31, 2025

sesquideus commented Jan 31, 2025

hugobuddel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hugobuddel commented Jan 31, 2025

sesquideus commented Jan 31, 2025

hugobuddel commented Feb 3, 2025

Choose a reason for hiding this comment