Karr/finalWorkflow #168

JenniferKarr · 2025-11-28T15:05:44Z

Basic implementation of the third workflow/recipes for ADI. A bit rough, but it produces the correct output.

sesquideus

From my side it looks fine -- I have a few minor reservations but that can be improved later.

sesquideus · 2025-11-28T15:14:00Z

metisp/pymetis/src/pymetis/dataitems/hci/appHci.py

+
+
+
+class SciCentred(BandSpecificMixin, CgrphSpecificMixin, ImageDataItem, abstract=True):


Currently this does not seem to be crgph-specific.

sesquideus · 2025-11-28T18:22:35Z

metisp/pymetis/src/pymetis/dataitems/hci/hci.py

+
+
+class LmAppSciCentred(BandLmMixin, CgrphAppMixin, SciCentred):
+    _name_template = r"{band}_APP_SCI_CENTRED"


If this is indeed something that can be parameterized by cgrph, the name template should most likely read r'{band}_{cgrph}_SCI_CENTRED

I don't yet fully understand how the abstraction works, so I might misunderstand.

The CgrphAppMixin implies that the class is partially specialized and thus that cgrph part of the template name is correctly substituted with (by?) APP.

But then I would expect the same with the band: the BandLmMixin is used, and the class is called LmAppSciCentred, so I think the template name should be LM_APP_SCI_CENTRED.

sesquideus · 2025-11-28T18:23:55Z

metisp/pymetis/src/pymetis/dataitems/hci/ifuHci.py

+from pymetis.classes.mixins.cgrph import CgrphSpecificMixin
+
+
+class IfuOffAxisPsfRaw(ImageDataItem, abstract=True):


Fully specialized data items should not be abstract (in fact that could be the definition and in that case we can dispense with abstract altogether).

hugobuddel

Good, almost the full workflow complete!

It seems that there are several small mistakes that need to be rectified. I probably have missed some too, so please check. There are also several duplicate classes, that I think is an accident.

I suggest to either create these classes automatically, or at least use a script to verify that they are correct.

Also, please reuse the names we already have. It is confusing to see the name of a task, and then have to guess which recipe it corresponds to. Reinventing names adds friction for no reason that I can discern.

Can you also please address the open comments on #167? You essentially resubmitted that PR for review, and apparently I now reviewed partially the same code twice.

Nevertheless, thanks for taking on this tedious task!

hugobuddel · 2025-11-29T17:53:31Z

metisp/pymetis/src/pymetis/dataitems/hci/hci.py

    pass


+class LmRavcCentroidTab(BandLmMixin, CgrphAppMixin,  CentroidTab):


I think this is exactly the same as the next class; did you mean

Suggested change

class LmRavcCentroidTab(BandLmMixin, CgrphAppMixin, CentroidTab):

class LmAppCentroidTab(BandLmMixin, CgrphAppMixin, CentroidTab):

hciApp.py was redundant; I temporarily forgot I had defined all the imaging data classes with the first HCI workflow. I have removed it.

hugobuddel · 2025-11-29T17:59:26Z

metisp/pymetis/src/pymetis/dataitems/hci/hci.py

+
+
+class LmAppSciCentred(BandLmMixin, CgrphAppMixin, SciCentred):
+    _name_template = r"{band}_APP_SCI_CENTRED"


I don't yet fully understand how the abstraction works, so I might misunderstand.

The CgrphAppMixin implies that the class is partially specialized and thus that cgrph part of the template name is correctly substituted with (by?) APP.

But then I would expect the same with the band: the BandLmMixin is used, and the class is called LmAppSciCentred, so I think the template name should be LM_APP_SCI_CENTRED.

hugobuddel · 2025-11-29T18:02:22Z

metisp/pymetis/src/pymetis/dataitems/hci/ifuHci.py

+class IfuOffAxisPsfRaw(ImageDataItem, abstract=True):
+
+    _name_template = r'IFU_OFF_AXIS_PSF_RAW'
+    _title_template = r"IFU RAVC sci calibration"


Is IfuOffAxisPsfRaw only applicable to RAVC? Could be

hugobuddel · 2025-11-29T18:06:44Z

metisp/pymetis/src/pymetis/dataitems/hci/ifuHci.py

+class IfuRavcSciThroughput(CgrphAppMixin, IfuSciThroughput):
+    pass
+
+class IfuCvcSciThroughput(CgrphAppMixin, IfuSciThroughput):
+    pass
+
+class IfuAppSciThroughput(CgrphAppMixin, IfuSciThroughput):
+    pass


These Mixin's are all for App, that doesn't seem right. Maybe

Suggested change

class IfuRavcSciThroughput(CgrphAppMixin, IfuSciThroughput):

pass

class IfuCvcSciThroughput(CgrphAppMixin, IfuSciThroughput):

pass

class IfuAppSciThroughput(CgrphAppMixin, IfuSciThroughput):

pass

class IfuRavcSciThroughput(CgrphRavcMixin, IfuSciThroughput):

pass

class IfuCvcSciThroughput(CgrphCvcMixin, IfuSciThroughput):

pass

class IfuAppSciThroughput(CgrphAppMixin, IfuSciThroughput):

pass

hugobuddel · 2025-11-29T18:07:39Z

metisp/pymetis/src/pymetis/dataitems/hci/ifuHci.py

+class IfuRavcSciCentred(CgrphAppMixin, IfuSciCentred):
+    pass
+
+class IfuRavcCentroidTab(CgrphAppMixin, IfuCentroidTab):
+    pass
+
+class IfuCvcSciCentred(CgrphAppMixin, IfuSciCentred):
+    pass
+
+class IfuCvcCentroidTab(CgrphAppMixin, IfuCentroidTab):
+    pass


Suggested change

class IfuRavcSciCentred(CgrphAppMixin, IfuSciCentred):

pass

class IfuRavcCentroidTab(CgrphAppMixin, IfuCentroidTab):

pass

class IfuCvcSciCentred(CgrphAppMixin, IfuSciCentred):

pass

class IfuCvcCentroidTab(CgrphAppMixin, IfuCentroidTab):

pass

class IfuRavcSciCentred(CgrphRavcMixin, IfuSciCentred):

pass

class IfuRavcCentroidTab(CgrphRavcMixin, IfuCentroidTab):

pass

class IfuCvcSciCentred(CgrphCvcMixin, IfuSciCentred):

pass

class IfuCvcCentroidTab(CgrphCvcMixin, IfuCentroidTab):

pass

hugobuddel · 2025-11-29T18:22:25Z

metisp/pymetis/src/pymetis/recipes/hci/metis_lm_adi_app.py

+            return {product_lmSciCalibrated, product_lmSciCentred, product_lmSciCentred, product_lmCentroidTable, product_lmSciHifilt, product_lmSciDerotatedPsfsub, product_lmSciDerotated, product_lmSciContrastRadprof, product_lmSciContrastAdi, product_lmSciThroughput, product_lmSciCoverage, product_lmSciSnr, product_lmSciPsfMedian}
+
+
+class MetisLmAppSciCalibrated(MetisRecipe):


Idem, here I would expect the name MetisLmAdiApp

hugobuddel · 2025-11-29T18:24:02Z

metisp/workflows/metis/metis_ifu_ravc_wkf.py

+ifu_ravc_post_task = (task('ifu_ravc_post')
+             .with_recipe('metis_ifu_adi_cgrph')


Place name the tasks like the recipes as much as feasible, otherwise everyone has to learn too many different names. The recipe, the recipe class, the edps task can all have names that are simply different forms of the same thing.

Suggested change

ifu_ravc_post_task = (task('ifu_ravc_post')

.with_recipe('metis_ifu_adi_cgrph')

ifu_adi_ravc_task = (task('ifu_adi_ravc_task')

.with_recipe('metis_ifu_adi_cgrph')

I renamed the tasks, and pushed the changes.

hugobuddel · 2025-11-29T18:27:19Z

metisp/pymetis/src/pymetis/dataitems/hci/appHci.py

+class CentroidTab(BandSpecificMixin, CgrphSpecificMixin, ImageDataItem, abstract=True):
+
+    _name_template = r'{band}_RAVC_CENTROID_TAB'
+    _title_template = r"{band} RAVC centroid tab"


I can't say that I fully understand the abstraction layer, but I would not expect RAVC here, because I wouldn't know where the CVC equivalent would go. Or does that not exist?

If it is cgrph-specific, it should naturally say r'{band}_{cgrph}_CENTROID_TAB'. If it is not, CgrphSpecificMixin should go away.

That is the thought I had too, either CgrphSpecificMixin and {cgrph} in the name, or neither.

The nomenclature is quite confusing though. I interpreted the term "cgrph-specific" as meaning "for a specific coronograph", e.g. "for the RAVC coronograph".

I just finished a better structure, in which this is no longer needed at all. Setting CgrphRavcMixin in the derived class will be enough.

hugobuddel · 2025-11-29T18:30:09Z

metisp/pymetis/src/pymetis/dataitems/hci/appHci.py

+class AdiCalibrated(BandSpecificMixin, CgrphSpecificMixin, ImageDataItem, abstract=True):
+
+    _name_template = r'{band}_RAVC_SCI_CALIBRATED'
+    _title_template = r"{band} RAVC sci calibration"


This class mentions RAVC, in fact, most of the classes in this file do. But this file is in the appHci.py file. I would expect APP classes in a file with that name. Maybe I don't understand things properly, at least it is confusing.

hugobuddel · 2025-11-29T18:32:44Z

metisp/pymetis/src/pymetis/dataitems/hci/appHci.py

+        'DET1.DATA': Image,
+    }
+
+class SciDerotated(BandSpecificMixin, CgrphSpecificMixin, ImageDataItem, abstract=True):


Most of these classes are now defined both in hci.py and in appHci.py. We could do that, but I think it would be better if we give the classes unique names.

Also, it seems the classes have the same _name_template, and I don't think that is allowed, because aren't those the unique identifiers of the class?

Or in case you actually need to use the same class in different places, you can just import them. (If that leads to circular imports, then you can refactor them by putting all the shared classes in a separate file.)

deleted superfluous file

JenniferKarr added 3 commits November 28, 2025 14:02

ifu hci workflow

a513413

fixed thing I unfixed

022afe5

functioning LM APP workflow. The final workflow (aside from persistence)

552ab70

JenniferKarr requested review from hugobuddel and sesquideus November 28, 2025 15:05

sesquideus approved these changes Nov 28, 2025

View reviewed changes

hugobuddel requested changes Nov 29, 2025

View reviewed changes

changed some task names

16cb2d4

deleted superfluous file




		class SciCentred(BandSpecificMixin, CgrphSpecificMixin, ImageDataItem, abstract=True):



		class LmAppSciCentred(BandLmMixin, CgrphAppMixin, SciCentred):
		_name_template = r"{band}_APP_SCI_CENTRED"

		from pymetis.classes.mixins.cgrph import CgrphSpecificMixin


		class IfuOffAxisPsfRaw(ImageDataItem, abstract=True):

		pass


		class LmRavcCentroidTab(BandLmMixin, CgrphAppMixin, CentroidTab):

	class LmRavcCentroidTab(BandLmMixin, CgrphAppMixin, CentroidTab):
	class LmAppCentroidTab(BandLmMixin, CgrphAppMixin, CentroidTab):

		return {product_lmSciCalibrated, product_lmSciCentred, product_lmSciCentred, product_lmCentroidTable, product_lmSciHifilt, product_lmSciDerotatedPsfsub, product_lmSciDerotated, product_lmSciContrastRadprof, product_lmSciContrastAdi, product_lmSciThroughput, product_lmSciCoverage, product_lmSciSnr, product_lmSciPsfMedian}


		class MetisLmAppSciCalibrated(MetisRecipe):

		ifu_ravc_post_task = (task('ifu_ravc_post')
		.with_recipe('metis_ifu_adi_cgrph')

Karr/finalWorkflow #168

Are you sure you want to change the base?

Karr/finalWorkflow #168

Uh oh!

Conversation

JenniferKarr commented Nov 28, 2025

Uh oh!

sesquideus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hugobuddel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants