Massive overhaul of core functionality #113

sesquideus · 2025-02-24T16:37:22Z

Sorry for the branch name, it got out of hand pretty quickly.

I tried to reimplement the loading part of the class hierarchy, so that it does not have to instantiate the RecipeImpl twice. As usual, the rabbithole went on and on... and we can now:

inspect and build the class before it is instantiated, even without a valid frameset;
split everything into working child classes (by detector, target, band...) much more easily
also automatically promote the class to a child based on the data
produce a working pyesorex --man-page <recipe> from the attributes of the Recipe, and without a valid frameset. No more writing and formatting the description yourself!
have a much more verbose as_dict to compare with the DRLD, including OCA keywords, algorithm (my long-term goal is to to recreate all DRLD boxes from the code, even if we don't actually do it, it's good to be able to)
get rid of the whole repetitive inputs |= {...}, as this is now done fully automatically with inspect (every PipelineInput in the InputSet gets instantiated and filled with data)
have much simpler and declarative class definitions, and also mixins make a bit more sense

Some downsides:

certain pyesorex errors are more cryptic / nonexistent, because it catches many exceptions, so if you screw up, suddenly your recipe does not exist anymore (requires some minor tweaks to your working pyesorex, no problem if you ˙pip install -e` it)
some things are defined implicitly and no longer collocated in the code, also IDE inspection has some problems
I noticed that many Inputs / Products are in fact the same (no shit Sherlock) and also keywords could probably be objects and not just strings, so I have to fix this too

Requires fixes to pyesorex (terminal size thing and better comparing for None) so it will not pass the tests right now.

hugobuddel · 2025-02-25T19:36:03Z

I keep smiling at the title of this PR in the email notifications. Maybe we should generate an image for each PR based on the title 🤓

sesquideus · 2025-02-25T20:00:21Z

Sorry to rain on your asteroid mining parade 😄

hugobuddel · 2025-03-02T12:39:23Z

The error is raised by from pymetis.tests.classes import BaseRecipeTest, BaseInputSetTest, BaseProductTest. The error is E ImportError: cannot import name 'BaseRecipeTest' from 'pymetis.tests.classes' (unknown location), and it seems to me that BaseRecipeTest is indeed not defined in pymetis.tests.classes .Maybe there should be a __init__.py in pymetis/tests/classes that imports BaseRecipeTest?

metisp/pymetis/src/pymetis/recipes/n_img/metis_n_img_flat.py

hugobuddel · 2025-03-02T12:48:14Z

metisp/pymetis/setup.cfg.in

@@ -1,6 +1,6 @@
 [metadata]
 name = pymetis
-version = @PACKAGE_VERSION@
+version = 0.0.1


The ESO build system might expect @PACKAGE_VERSION@ here. I suppose everything would still work with a specific version though.

I think the idea is to have only one place where the version number is defined, which canonically is

METIS_Pipeline/metisp/configure.ac

Line 3 in d31dfd2

AC_INIT([METIS Instrument Pipeline], [0.0.1], [https://support.eso.org],

. That version is then propagated to here by autotools or cmake.

Maybe it is easier for our development if we (also) define the version explicitly here, but it should be a conscious decision that means that we then have an extra location to update the version number.

I'm impartial in what we decide, so I'm not for or against this change; I'm just noting we should be aware of what it means.

(I personally think that the 'current version' should be derived from the git tag/branch/hash, like astropy, but I don't actually suggest doing that now.)

hugobuddel · 2025-03-02T12:49:43Z

metisp/pymetis/src/pymetis/classes/inputs/base.py

Two return statements in a row seems like a mistake.

Forgot about this from some late night debugging session. Right now I'd prefer the unmodified version, since it explicitly shows what the name of the parameter is -- and how it is propagated to outputs.

hugobuddel · 2025-03-02T12:57:43Z

metisp/pymetis/src/pymetis/classes/inputs/common.py

+class FluxstdCatalogInput(SinglePipelineInput):
    _title: str = "flux standard star catalogue table"


Just a note that we made a distinction between "tables" and "catalogs". A 'catalog' is a list of sources (e.g. stars) with their attributes, it is encoded as a FITS table extension. A 'table' contains something else, e.g. a list of offsets for each detector, and we have not decided how it should be encoded. For example, a table with just four numbers can perhaps be better encoded as four FITS headers, because that makes it queryable through the archive. The term "catalog[ue] table" is never used in the DRLD in order to prevent confusion.

My bad. This was probably autogenerated by some PyCharm AI voodoo and I failed to notice.

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

hugobuddel · 2025-03-02T13:09:23Z

metisp/pymetis/src/pymetis/classes/inputs/inputset.py

+        cut_input = re.compile(r'Input$')
+
+        # Now iterate over all refined Inputs, instantiate them and feed them the frameset to filter.
+        for (name, input_type) in inspect.getmembers(self.__class__,


Usually, when I want to use inspect, I'm doing something wrong. So this line triggers me, and there is not enough information here to take away my fear that we are doing something wrong here too. Maybe it is fine though.

Perhaps you can iterate over self.__class__.__dict__.items() instead? But maybe that won't work for some reason? It would still be a bit hacky, but somewhat less, IMHO.

I think inspect is also pretty heavy, and maybe implementation dependent (not sure)?

I am not sure. There are many possible approaches, this felt cleanest to me. Also we can go fully explicit.

What I both like and dislike the most is that it happens fully automatically. It can lead to disgusting bugs (like failing to match the name of the overridden class, which results in extra inputs (but that can be caught by looking at the man page)). Also code collocation is horrendous -- all the magic happens way too high in the hierarchy.

hugobuddel · 2025-03-02T13:18:44Z

metisp/pymetis/src/pymetis/classes/inputs/inputset.py

+        for (name, input_type) in inspect.getmembers(self.__class__,
+                                                     lambda x: inspect.isclass(x) and issubclass(x, PipelineInput)):
+            inp = input_type(frameset)
+            # FixMe: very hacky for now: determine the name of the instance from the name of the class


Just run with it! I can understand it, I think:

(?<!^)(?=[A-Z]) means: look-back to determine we are not at the start, and then look-here/ahead but don't consume (?) and this is a capital. If so, insert an underscore. Before all that, remove "Input", and afterwards make lowercase. So this is debugable.

hugobuddel · 2025-03-02T13:30:55Z

metisp/pymetis/src/pymetis/classes/products/product.py

+
+    # Use this regex to verify that the product tag is correct.
+    # This base version only verifies it is ALL_CAPS_WITH_UNDERSCORES
+    _regex_tag: re.Pattern = re.compile(r"^[A-Z0-9]+[A-Z0-9_]+[A-Z0-9]+$")


Is a tag starting with a number going to work? I can imagine it leading to problems, not sure.

I guess it is (at least I don't see a reason why it should not work). But it might be better to just forbid that.

hugobuddel · 2025-03-02T13:33:52Z

metisp/pymetis/src/pymetis/classes/recipes/impl.py

@@ -181,3 +183,21 @@ def _create_dummy_image() -> cpl.core.Image:
    @property
    def valid_frames(self) -> cpl.ui.FrameSet:
        return self.inputset.valid_frames
+
+    def _dispatch_child_class(self) -> type["MetisRecipeImpl"]:


Supersweet!

hugobuddel

Looking good!

hugobuddel · 2025-03-02T14:08:50Z

Perhaps we should update the cached files? Or maybe wait till pyesorex 1.0.2 is released (which should be next week)

sesquideus · 2025-03-02T14:24:10Z

Perhaps we should update the cached files? Or maybe wait till pyesorex 1.0.2 is released (which should be next week)

No need to hurry now. I would like to fix the tests first anyway.

sesquideus · 2025-03-02T22:21:20Z

OK, I can reproduce the same test failures with my local machine on pyesorex main. And with the latest fixes it might now pass.

Updating pyesorex version requirement to 1.0.2b0 in the feature branch

sesquideus added 15 commits February 18, 2025 18:57

Merge branch 'mb/loose-ends' into mb/extra-tests

ae41d22

Returning to class attributes for Products

a9833cf

Experimenting with class attributes

1e189b7

Called back home early

2a9b41f

Hacking around with classes

2a01459

More deep digging

782a274

Converting happily

b56876b

Breaking and fixing things until it works again

7161c9c

Overhaul of class promotion mechanism

8e92789

All non-EDPS tests passing

d56479b

Some inputs are not required for now

5fc8561

Reorganized tests, added keyword and algorithm fields

e72968f

Added more metadata tests

c4ce9aa

Added N recipes, refactored hierarchy for similar LM/N recipes

11bdace

Work in progress but I have to run home

47de469

sesquideus changed the title ~~Massive overhaul of ore functionality~~ Massive overhaul of core functionality Feb 25, 2025

sesquideus added 12 commits February 25, 2025 21:50

Restructured and added more descriptive metadata

b78ee1a

Updated metis_ifu_telluric too

8fbce46

Better file layout, fixed all missing metadata

09ac0f2

A little more deduplication

15c3d48

Broke everything up again

8481975

Fixed overnight bugs

580e59d

Removed an extra target

2ede8d6

Added metis_n_img_restore + tests

6107fd0

Removed forgotten target assignment

da59439

Fixed a stupid class naming bug

f949999

And also from tests

c08b54a

Properly promoting IFU reduce

5758985

sesquideus added 5 commits February 28, 2025 14:28

Reintroducing Mixins for detector/band/target

c54dd17

The fight goes ever on

a910c9b

Added type annotations

ea83585

Tidying up a little bit

8267123

Apparently __init__.py files were ignored

0f2078b

hugobuddel reviewed Mar 2, 2025

View reviewed changes

metisp/pymetis/src/pymetis/recipes/n_img/metis_n_img_flat.py Outdated Show resolved Hide resolved

Added one more missing __init__.py

841da4b

hugobuddel reviewed Mar 2, 2025

View reviewed changes

Update metisp/pymetis/src/pymetis/recipes/n_img/metis_n_img_flat.py

5652771

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

hugobuddel reviewed Mar 2, 2025

View reviewed changes

hugobuddel mentioned this pull request Mar 2, 2025

Cy/issue 0033 #88

Merged

hugobuddel reviewed Mar 2, 2025

View reviewed changes

hugobuddel approved these changes Mar 2, 2025

View reviewed changes

Removed __init__.py from .gitignore

21c2599

Corrected the input title

04f428f

sesquideus and others added 6 commits March 3, 2025 11:28

Minor fixes from Hugo

c574741

Added a tutorial to metis_det_dark

2880398

Minor fixes to the tutorial comments

0286ca7

Minor type hints

37059f5

Refactored description builder

8639d32

Update requirements.txt

67dec20

Updating pyesorex version requirement to 1.0.2b0 in the feature branch

sesquideus merged commit 53e6a91 into main Mar 10, 2025
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Massive overhaul of core functionality #113

Massive overhaul of core functionality #113

sesquideus commented Feb 24, 2025

hugobuddel commented Feb 25, 2025

sesquideus commented Feb 25, 2025 •

edited

Loading

hugobuddel commented Mar 2, 2025

hugobuddel Mar 2, 2025

hugobuddel Mar 2, 2025

sesquideus Mar 2, 2025

hugobuddel Mar 2, 2025

sesquideus Mar 2, 2025

hugobuddel Mar 2, 2025

sesquideus Mar 2, 2025

hugobuddel Mar 2, 2025

hugobuddel Mar 2, 2025

sesquideus Mar 2, 2025

hugobuddel Mar 2, 2025

hugobuddel left a comment

hugobuddel commented Mar 2, 2025

sesquideus commented Mar 2, 2025

sesquideus commented Mar 2, 2025

		class FluxstdCatalogInput(SinglePipelineInput):
		_title: str = "flux standard star catalogue table"

Massive overhaul of core functionality #113

Massive overhaul of core functionality #113

Conversation

sesquideus commented Feb 24, 2025

hugobuddel commented Feb 25, 2025

sesquideus commented Feb 25, 2025 • edited Loading

hugobuddel commented Mar 2, 2025

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 left a comment

Choose a reason for hiding this comment

hugobuddel commented Mar 2, 2025

sesquideus commented Mar 2, 2025

sesquideus commented Mar 2, 2025

sesquideus commented Feb 25, 2025 •

edited

Loading