Additional hotfix for the removed currsys #368
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## dev_master #368 +/- ##
==============================================
- Coverage 74.60% 74.35% -0.25%
==============================================
Files 57 56 -1
Lines 7918 7835 -83
==============================================
- Hits 5907 5826 -81
+ Misses 2011 2009 -2 ☔ View full report in Codecov by Sentry. |
|
Did your run also the tests of the irdb with this branch of ScopeSim installed? |
|
Currently do that. OSIRIS still seems to have a problem with 2 tests, but the others are now failing due to an astropy error I'll mark this as a draft PR until the irdb tests are fixed |
astronomyk
changed the title
| @@ -21,11 +23,18 @@ def __init__(self, **kwargs): | |||
| self.meta.update(params) | |||
| self.meta.update(kwargs) | |||
|
|
|||
| tbl = from_currsys(self.table, self.cmds) | |||
| self.surfaces = rad_utils.make_surface_dict_from_table(tbl) | |||
There was a problem hiding this comment.
It seems that make_surface_dict_from_table() is now redundant and perhaps can be deleted?
There was a problem hiding this comment.
Yes it can. I did do that but get my branches confused, and they forgot to re-delete it from this branch. Thanks 👍
There was a problem hiding this comment.
The same can be done to make_surface_from_row
I got these about the FAILED OSIRIS/tests/test_compiles.py::TestOsirisLongSlitCompiles::test_run_lss_simulation - TypeError: scopesim.effects.spectral_trace_list.SpectralTraceList() got multiple values for keyword argument 'cmds' And this one seems more insiduous: FAILED WFC3/test_wfc3/test_full_package_wfc3_ir.py::TestObserveOpticalTrain::test_actually_produces_stars - assert 1032.9545687957511 == 7965343.479197175 ± 8.0e+04 |
|
Ok, looks like the OSIRIS tests are passing locally now. |
|
@hugobuddel , gotta chuff off now, but locally everything is fine except for the WFC3 test. That one might need a bit more looking into. Question is, if we think its just an old test on a very old package that doesn't like something now .... Hmmm |
|
@astronomyk I added some commits to your branch, and it seems to work now, so I'll merge it. The problem with WFC3 was that So I fixed that, and then the The CI passes, however on my machine the tests randomly fail. And with random I mean that they fail depending on the order in which they are ran. If I test with If they fail, it is with setting up the test: ...
scopesim/tests/tests_effects/test_ReferencePixelBorder.py .....
scopesim/tests/tests_effects/test_ter_curves_utils.py E
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> traceback >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
self = <unittest.mock._patch_dict object at 0x7f8d4ccb9b50>
def _patch_dict(self):
values = self.values
if isinstance(self.in_dict, str):
self.in_dict = pkgutil.resolve_name(self.in_dict)
in_dict = self.in_dict
clear = self.clear
try:
> original = in_dict.copy()
E AttributeError: 'UserCommands' object has no attribute 'copy'
[...]/lib/python3.11/unittest/mock.py:1872: AttributeError
During handling of the above exception, another exception occurred:
@pytest.fixture(scope="function")
def no_file_error():
"""Patch currsys to avoid missing file error."""
patched = {"!SIM.file.error_on_missing_file": False}
> with patch.dict("scopesim.rc.__currsys__", patched):
scopesim/tests/conftest.py:92:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
[...]/lib/python3.11/unittest/mock.py:1860: in __enter__
self._patch_dict()
[...]/lib/python3.11/unittest/mock.py:1877: in _patch_dict
for key in in_dict:
scopesim/commands/user_commands.py:265: in __getitem__
return self.cmds.__getitem__(item)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = NestedMapping({'SIM': {'spectral': {'wave_min': 0.7, 'wave_mid': 1.6, 'wave_max': 2.5, 'wave_unit': 'um', 'spectral_bi...em', 'yamls': ['MICADO_Standalone_RO.yaml'], 'RO': {'temperature': '!ATMO.temperature', 'element_name': 'default_ro'}})
key = 0
def __getitem__(self, key: str):
"""x.__getitem__(y) <==> x[y]."""
if isinstance(key, str) and key.startswith("!"):
key_chunks = self._split_subkey(key)
entry = self.dic
for chunk in key_chunks:
self._guard_submapping(
entry, key_chunks[:key_chunks.index(chunk)], "get")
try:
entry = entry[chunk]
except KeyError as err:
raise KeyError(key) from err
return entry
> return self.dic[key]
E KeyError: 0
../astar-utils/astar_utils/nested_mapping.py:76: KeyError |
|
70 uses of $ rg "from_currsys\([^,]*\)"
scopesim/tests/test_utils_functions.py
177: assert utils.from_currsys("!SIM.random.seed") is None
180: assert utils.from_currsys(["!SIM.random.seed"]*3)[2] is None
183: assert utils.from_currsys(np.array(["!SIM.random.seed"]*2))[1] is None
186: assert utils.from_currsys({"seed": "!SIM.random.seed"})["seed"] is None
191: result = utils.from_currsys("!SIM.sub_pixel.flag")
198: assert utils.from_currsys(tbl["seeds2"][1]) is None
203: result = utils.from_currsys("!SIM.sub_pixel.fraction")
215: assert not from_currsys(opt["slit_wheel"].include)
221: assert from_currsys(opt["slit_wheel"].include)
227: assert from_currsys(opt["ifu_spectral_traces"].include)
234: # assert from_currsys(opt["slit_wheel"].include)
scopesim/utils.py
484: # filename = from_currsys(filename)
517: if from_currsys("!SIM.file.error_on_missing_file"):
880: item = from_currsys(item)
scopesim/tests/tests_effects/test_NonCommonPathAberation.py
115: wave_min = from_currsys("!SIM.spectral.wave_min")
116: wave_max = from_currsys("!SIM.spectral.wave_max")
scopesim/tests/tests_effects/test_DetectorModePropertiesSetter.py
68: assert from_currsys(key_name) == basic_dmps.mode_properties["fast"][key_name]
75: assert from_currsys("!OBS.detector_readout_mode") == "slow"
scopesim/tests/tests_effects/test_AutoExposure.py
65: out_dit = from_currsys("!OBS.dit")
66: out_ndit = from_currsys("!OBS.ndit")
79: out_dit = from_currsys("!OBS.dit")
91: out_dit_1 = from_currsys("!OBS.dit")
92: out_ndit_1 = from_currsys("!OBS.ndit")
99: out_dit_2 = from_currsys("!OBS.dit")
100: out_ndit_2 = from_currsys("!OBS.ndit")
113: dit_1 = from_currsys("!OBS.dit")
114: ndit_1 = from_currsys("!OBS.ndit")
120: dit_2 = from_currsys("!OBS.dit")
121: ndit_2 = from_currsys("!OBS.ndit")
134: dit = from_currsys("!OBS.dit")
135: ndit = from_currsys("!OBS.ndit")
scopesim/reports/rst_utils.py
309: path = from_currsys(rc.__config__["!SIM.reports.latex_path"])
348: path = from_currsys(rc.__config__["!SIM.reports.rst_path"])
scopesim/optics/fov.py
370: spline_order = from_currsys("!SIM.computing.spline_order")
396: if from_currsys(self.meta["sub_pixel"]):
496: if from_currsys(self.meta["sub_pixel"]):
508: # pixel_area = from_currsys(self.meta["pixel_scale"]) ** 2 # float [arcsec2]
562: spline_order = from_currsys("!SIM.computing.spline_order")
566: wave_unit = u.Unit(from_currsys("!SIM.spectral.wave_unit"))
569: from_currsys("!SIM.spectral.spectral_bin_width")) * wave_unit
616: area = from_currsys(self.meta["area"]) # u.m2
scopesim/optics/optical_train.py
326: wave_unit = u.Unit(from_currsys("!SIM.spectral.wave_unit"), self.cmds)
scopesim/effects/metis_lms_trace_list.py
201: dwave = from_currsys("!SIM.spectral.spectral_bin_width")
234: cubehdr["INSMODE"] = from_currsys(self.meta["element_name"])
235: cubehdr["WAVELEN"] = from_currsys(self.meta["wavelen"])
257: lam = from_currsys(self.meta["wavelen"])
335: lam0 = from_currsys(self.meta["wavelen"])
462: f"{from_currsys(self.meta['wavelen'])} um : "
531: filename = find_file(from_currsys(filename))
570: wavelen = from_currsys(kwargs["wavelen"])
scopesim/effects/ter_curves.py
154: # bg_cell_width = from_currsys(self.meta["bg_cell_width"])
885: transmission = from_currsys(transmission)
928: for name in from_currsys(self.meta["adc_names"]):
950: curradc = from_currsys(self.meta["current_adc"])
scopesim/effects/ter_curves_utils.py
55: filter_name = from_currsys(filter_name)
scopesim/effects/psf_utils.py
114: spline_order = utils.from_currsys("!SIM.computing.spline_order")
scopesim/effects/obs_strategies.py
69: chop_offsets = from_currsys(self.meta["chop_offsets"])
70: nod_offsets = from_currsys(self.meta["nod_offsets"])
75: pixel_scale = float(from_currsys(self.meta["pixel_scale"]))
scopesim/optics/fov_utils.py
164: spline_order = from_currsys("!SIM.computing.spline_order")
scopesim/effects/rotation.py
38: rotations = from_currsys(self.meta["rotations"])
scopesim/effects/fits_headers.py
149: E.g: ``from_currsys('!ATMO.temperature')`` will resolve to a float
352: value = from_currsys(value)
scopesim/optics/image_plane.py
112: sub_pixel = from_currsys("!SIM.sub_pixel.flag")
114: spline_order = from_currsys("!SIM.computing.spline_order")I suppose those should all get a |
This looks very similar to some errors I saw from time to time recently. Usually occurs when some tests mess with the |
teutoburg
changed the title
|
I took the liberty to remove the "DRAFT" from this PR's title. We should probably make use of GitHub's existing "Draft PR" feature. Which most of us do already, but not all. If you're reading this, you know who you are 😜 |
It is literally more work to add the word DRAFT to the title than press the button. I've created a pictorial guide in #357 (comment) . (I deliberately kept the title.) |
Yeah I figured that must be it. I did not recall seeing this specific error before, but I probably did. I thought maybe now we could remove this But the shortterm fix would be to add the |
|
We are still not there though, demo_rectify_traces.ipynb fails as can be seen in last nights run of ScopeSim_Data: ---------------------------------------------------------------------------
KeyError Traceback (most recent call last)
Cell In[13], line 1
----> 1 rectified = tracelist.rectify_traces(readout, -4.0, 4.0)
File /tmp/tmp.MmHn0JRFMn/ScopeSim/scopesim/effects/spectral_trace_list.py:301, in SpectralTraceList.rectify_traces(self, hdulist, xi_min, xi_max, interps, **kwargs)
298 except ValueError:
299 filter_name = from_currsys("!OBS.filter_name_fw1", self.cmds)
--> 301 filtcurve = FilterCurve(
302 filter_name=filter_name,
303 filename_format=from_currsys("!INST.filter_file_format", self.cmds))
304 filtwaves = filtcurve.table["wavelength"]
305 filtwave = filtwaves[filtcurve.table["transmission"] > 0.01]
File /tmp/tmp.MmHn0JRFMn/ScopeSim/scopesim/effects/ter_curves.py:403, in FilterCurve.__init__(self, **kwargs)
400 if not np.any([key in kwargs for key in ["filename", "table",
401 "array_dict"]]):
402 if "filter_name" in kwargs and "filename_format" in kwargs:
--> 403 filt_name = from_currsys(kwargs["filter_name"], kwargs["cmds"])
404 file_format = from_currsys(kwargs["filename_format"], kwargs["cmds"])
405 kwargs["filename"] = file_format.format(filt_name)
KeyError: 'cmds'I do recall I did run FWIW, I think we should reduce the use of |
|
Oh FFS! |
I hopefully fixed that in 40cbb7e which I accidentally committed directly to |
|
ScopeSim_Data now runs successfully, so everything is probably working again https://github.com/AstarVienna/ScopeSim_Data/actions |
Indeed I saw your visual guide, and found it hilarious. And even more hilarious that the exact same thing it tried to address was done again at the very next opportunity. I know I know, I'll stop bullying my supervisor now 😝 |
Problem:
By resolving bang-strings in the
initmethod ofSurfaceList, there was no way to keep referenced-temperature keys for individual surfaces in their bang-string form until the time when they are actually needed. E.g. theMORFEOmirror temperature is set to reference theATMO.temperaturekey. Instead, all bang-strings were resolved during initialisation, killing this dynamic referencingSolution:
This is now fixed by specific code in the
SurfaceList.__init__method which resolves only the filename column of a surface list input table. (see scopesim/effects/surface_list.py line 36). This was achieved by incorporating the code from the smallradiometry_utilsfunctions (e.g.rad_utils.make_surface_dict_from_table) directly into theSurfaceList.__init__method.Additional changes
Years ago I incorporated the functional parts of RadiometryTable into SurfaceList, but never deleted the RadiometryTable code. This commit also deletes this old unused code and the test suite associated with it.
Additionally the
SpectralSurfaceobject is now also passed the.cmdsobject, even though it is not anEffectobject. It does however provide integral functionality toTERCurves(all the radiometric values like throughput and emission) and so it stands to reason that it might also need to resolve some!-strings on the fly.Locally this branch is green for me. Hence the pull request. I *hope that this fixes some of the bugs found by @hugobuddel in the IRDB