Fixes needed for IFU/LMS mode #376

hugobuddel · 2024-02-26T19:39:22Z

This adds the fixes required to get the IFU / LMS mode working again as described in #374 . It also contains some extra currsys fixes from #372 .

codecov · 2024-02-26T19:42:03Z

Codecov Report

Attention: Patch coverage is 91.66667% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 74.31%. Comparing base (d786495) to head (671b4f4).

Files	Patch %	Lines
scopesim/effects/metis_lms_trace_list.py	71.42%	2 Missing ⚠️
scopesim/optics/optical_train.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff               @@
##           dev_master     #376      +/-   ##
==============================================
+ Coverage       74.29%   74.31%   +0.02%     
==============================================
  Files              56       56              
  Lines            7818     7828      +10     
==============================================
+ Hits             5808     5817       +9     
- Misses           2010     2011       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

hugobuddel · 2024-02-26T21:02:49Z

BTW, I did not run the IRDB tests :-P

hugobuddel · 2024-02-26T22:08:44Z

The irdb tests run fine, but I should really also test the notebooks, so made it a draft.. But maybe we should go for #377 directly. All after #375 perhaps.

teutoburg

All this meta and cmds stuff might probably soon change again as the next episode in the ChainMap story. At which point I might also tackle these _class_params. But it's good if things (that is, the IFU/LMS mode) already works before that, so this is useful. But maybe we'll do #375 first. Or maybe this: first #375, then rebase #377 onto the resulting dev_master (which will include the changes here), and then once it passes just merge that, and close this...

hugobuddel · 2024-02-26T23:46:32Z

All this meta and cmds stuff might probably soon change again as the next episode in the ChainMap story.

All of this and #377 is kinda orthogonal to the ChainMap story. Because we still want this hierarchy of currsys and UserCommands (and apparently _class_params).

At which point I might also tackle these _class_params. But it's good if things (that is, the IFU/LMS mode) already works before that, so this is useful. But maybe we'll do #375 first. Or maybe this: first #375, then rebase #377 onto the resulting dev_master (which will include the changes here), and then once it passes just merge that, and close this...

It's fine to do #375 first; you were first and I don't want to break your branch.

I'd rather merge dev_master into #377 than rebase it, but potato potato.

teutoburg · 2024-02-27T18:12:39Z

I'd rather merge dev_master into #377 than rebase it, but potato potato.

Sure, whatever grinds your gears 🙃

astronomyk · 2024-02-28T13:40:16Z

scopesim/effects/metis_lms_trace_list.py

+        self.meta = copy.copy(self._class_params)
+        assert "grat_spacing" in self.meta, "grat_spacing is missing from self.meta 1"
+        super().__init__(**kwargs)
+        assert "grat_spacing" in self.meta, "grat_spacing is missing from self.meta 2"


I assume these assert statements are fine for now, and because this whole class is METIS specific, and nobody is going to screw too much with the current status quo in the METIS IRDB, these will not affect anything down the line. Or?

These asserts should not affect anything. I kept them in because maybe they are a canary that could indicate that something is wrong. What could go wrong is that someone changes __init__() of one of the super classes (e.g. Effect) to delete everything in self.meta. This will highlight if that happens.

astronomyk · 2024-02-28T13:45:43Z

scopesim/effects/psf_utils.py



-def rescale_kernel(image, scale_factor, spline_order=None):
+def rescale_kernel(image, scale_factor, spline_order=None, cmds=None):


I appreciate why this is here, but I would prefer to keep cmds out of the utils functions. Maybe it's just me being pedantic for no reason, but the util function should be able to be called by anything, even if a cmds object doesn't exist.

I would suggest deleting the call to currsys instead and forcing the offending Effect object to resolve the !-string before calling rescale_kernel

Suggested change

def rescale_kernel(image, scale_factor, spline_order=None, cmds=None):

def rescale_kernel(image, scale_factor, spline_order):

astronomyk · 2024-02-28T13:48:20Z

scopesim/effects/psf_utils.py


-def rescale_kernel(image, scale_factor, spline_order=None):
+def rescale_kernel(image, scale_factor, spline_order=None, cmds=None):
    if spline_order is None:


Suggested change

if spline_order is None:

astronomyk · 2024-02-28T13:48:31Z

scopesim/effects/psf_utils.py

+def rescale_kernel(image, scale_factor, spline_order=None, cmds=None):
    if spline_order is None:
-        spline_order = utils.from_currsys("!SIM.computing.spline_order")
+        spline_order = utils.from_currsys("!SIM.computing.spline_order", cmds=cmds)


Suggested change

spline_order = utils.from_currsys("!SIM.computing.spline_order", cmds=cmds)

OK, if we remove this here, then it is not necessary to add cmds as an argument to rescale_kernel(). I'll update the code in psf.py to get the spline order before calling rescale_kernel().

Note that you can comment on multiple lines at a time by "dragging the plus down" next to the lines in the diff view.

astronomyk · 2024-02-28T13:51:31Z

scopesim/effects/psfs.py

        wave = from_currsys(self.meta["wavelength"], self.cmds)
        if isinstance(wave, str) and wave in tu.FILTER_DEFAULTS:
-            wave = tu.get_filter_effective_wavelength(wave)
+            wave = tu.get_filter_effective_wavelength(wave, cmds=self.cmds)


The same here as above. I feel it would be better to force the Effect object to resolve what it needs before calling a util function

Suggested change

wave = tu.get_filter_effective_wavelength(wave, cmds=self.cmds)

wave = tu.get_filter_effective_wavelength(wave)

astronomyk · 2024-02-28T13:54:52Z

scopesim/effects/ter_curves_utils.py



-def get_filter_effective_wavelength(filter_name):
+def get_filter_effective_wavelength(filter_name, cmds=None):


Same as before, lets delete the currsys call

Suggested change

def get_filter_effective_wavelength(filter_name, cmds=None):

def get_filter_effective_wavelength(filter_name):

astronomyk · 2024-02-28T14:02:14Z

scopesim/effects/ter_curves_utils.py

+def get_filter_effective_wavelength(filter_name, cmds=None):
    if isinstance(filter_name, str):
-        filter_name = from_currsys(filter_name)
+        filter_name = from_currsys(filter_name, cmds=cmds)


Suggested change

filter_name = from_currsys(filter_name, cmds=cmds)

assert FILTER_DEFAULTS.get(filter_name), f"{filter_name} not found in FILTER_DEFAULTS"

astronomyk

Thanks for all these fixes @hugobuddel !

One comment on the background philosophy for getting rid of the cursed from_currsys. In general, I'd like to keep cmds out of the util function as much as possible. The buck should stop with the Effect objects.

In the next iteration, I plan to delete the from_currsys function completely, as all these resolution should theoretically also be possible now with the inbuilt functionality of the chainmaps. I.e. the following should return a resolved bang-string: self.cmds["reference_to_a_bang_string"].

And a step further would be to make the self.meta (or even params) the upper most level of the chain-map (a level-3 child? @teutoburg will know), and allowing all resolutions to happen on the fly internally, so that we will never actually need explicitly force a resolution

hugobuddel · 2024-02-28T14:32:48Z

Thanks for all these fixes @hugobuddel !

Note that #377 expands on this PR. I added your suggestions to that branch, as I think it is easier to just merge #377 directly and close this one.

One comment on the background philosophy for getting rid of the cursed from_currsys. In general, I'd like to keep cmds out of the util function as much as possible. The buck should stop with the Effect objects.

OK that sounds fair. Many of these utility functions are only called by a single Effect, so they could just as well be part of that class though.

In the next iteration, I plan to delete the from_currsys function completely, as all these resolution should theoretically also be possible now with the inbuilt functionality of the chainmaps. I.e. the following should return a resolved bang-string: self.cmds["reference_to_a_bang_string"].

Yes, in #377 I've made that change already wherever I needed to change the code anyway.

And a step further would be to make the self.meta (or even params) the upper most level of the chain-map (a level-3 child? @teutoburg will know), and allowing all resolutions to happen on the fly internally, so that we will never actually need explicitly force a resolution

That sounds good.

Fixes needed for IFU/LMS mode

ba9df7a

hugobuddel added 4 commits February 26, 2024 21:07

Resolve !-string filenames early, but do not actually look for the file.

2522e3a

Some other currsys related changes

1c27b55

Check whether file exists before loading it

710636a

forgot a cmds

671b4f4

hugobuddel requested a review from teutoburg February 26, 2024 20:47

This was referenced Feb 26, 2024

Do not set user commands as rc.__currsys__ #377

Merged

Fix METIS LMS / IFU mode AstarVienna/irdb#156

Merged

hugobuddel marked this pull request as draft February 26, 2024 22:07

teutoburg reviewed Feb 26, 2024

View reviewed changes

hugobuddel marked this pull request as ready for review February 27, 2024 07:35

astronomyk reviewed Feb 28, 2024

View reviewed changes

astronomyk requested changes Feb 28, 2024

View reviewed changes

hugobuddel merged commit 88ee6d6 into dev_master Feb 29, 2024

hugobuddel deleted the hb/lms branch February 29, 2024 21:11



		def rescale_kernel(image, scale_factor, spline_order=None):
		def rescale_kernel(image, scale_factor, spline_order=None, cmds=None):

	wave = tu.get_filter_effective_wavelength(wave, cmds=self.cmds)
	wave = tu.get_filter_effective_wavelength(wave)



		def get_filter_effective_wavelength(filter_name):
		def get_filter_effective_wavelength(filter_name, cmds=None):

	filter_name = from_currsys(filter_name, cmds=cmds)
	assert FILTER_DEFAULTS.get(filter_name), f"{filter_name} not found in FILTER_DEFAULTS"

Fixes needed for IFU/LMS mode #376

Fixes needed for IFU/LMS mode #376

Uh oh!

Conversation

hugobuddel commented Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hugobuddel commented Feb 26, 2024

Uh oh!

hugobuddel commented Feb 26, 2024

Uh oh!

teutoburg left a comment

Choose a reason for hiding this comment

Uh oh!

hugobuddel commented Feb 26, 2024

Uh oh!

teutoburg commented Feb 27, 2024

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!

astronomyk left a comment

Choose a reason for hiding this comment

Uh oh!

hugobuddel commented Feb 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hugobuddel commented Feb 26, 2024 •

edited

Loading

codecov bot commented Feb 26, 2024 •

edited

Loading