Added missing cmds to currsys calls in scopesim codebase #372
Conversation
astronomyk
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev_master #372 +/- ##
==============================================
+ Coverage 74.26% 74.28% +0.01%
==============================================
Files 56 56
Lines 7811 7815 +4
==============================================
+ Hits 5801 5805 +4
Misses 2010 2010 ☔ View full report in Codecov by Sentry. |
|
I'll go ahead and merge this. Locally everything is green in ScopeSim and IRDB - except for the 3 logging test. I'll leave these up to @teutoburg to fix ;) |
It seems like you missed some. Using the same ripgrep command as in #368, manually removing tests and comments: $ rg "from_currsys\([^,]*\)"
scopesim/utils.py
517: if from_currsys("!SIM.file.error_on_missing_file"):
880: item = from_currsys(item)
scopesim/optics/fov.py
566: wave_unit = u.Unit(from_currsys("!SIM.spectral.wave_unit"), self.cmds)
569: from_currsys("!SIM.spectral.spectral_bin_width"), self.cmds) * wave_unit
scopesim/optics/optical_train.py
326: wave_unit = u.Unit(from_currsys("!SIM.spectral.wave_unit"), self.cmds)
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/effects/ter_curves.py
875: transmission = from_currsys(transmission)
918: for name in from_currsys(self.meta["adc_names"]):
940: 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") |
|
Note that some of these seem to indicate bigger problems, because they appear to indicate that you added I take it that you did run the full irdb suite, including notebooks to test these changes before merging them. This code is now obviously broken, but apparently it is not used anywhere? |
| def __init__(self, **kwargs): | ||
| super().__init__(**kwargs) |
There was a problem hiding this comment.
Why did you undo these changes? It is better to make the cmds argument explicit and limit the use of kwargs to arguments you don't expect.
There was a problem hiding this comment.
Yep. I get that. But this ends in a bigger oroject of making sure every effect object has the explicit argument cmds in the init call
| fname = str(path).format(name) | ||
| self.trace_lists[name] = SpectralTraceList(filename=fname, | ||
| name=name, | ||
| cmds=self.cmds, |
There was a problem hiding this comment.
Idem, it is better to be explicit, why did you remove this?
There was a problem hiding this comment.
This was removed because cmds was also found in the kwargs. So to avoid doubling up on passing arguments, I left everything in kwargs
| fov_waveset = np.arange( | ||
| self.meta["wave_min"].value, self.meta["wave_max"].value, | ||
| from_currsys("!SIM.spectral.spectral_bin_width")) * wave_unit | ||
| from_currsys("!SIM.spectral.spectral_bin_width"), self.cmds) * wave_unit |
There was a problem hiding this comment.
Thanks. Curious.
| # 1. Make waveset and canvas cube (area, bin_width are applied at end) | ||
| # TODO: Why is this not self.waveset? What's different? | ||
| wave_unit = u.Unit(from_currsys("!SIM.spectral.wave_unit")) | ||
| wave_unit = u.Unit(from_currsys("!SIM.spectral.wave_unit"), self.cmds) |
|
Indeed I ran all the scopesim and IRDB tests, but didn't check the notebooks. Everything was green locally and on github actions, except for the logging tests. |
I did not run all the notebooks either. I think they might all work. The now-broken code is in the LMS parts, in |
As above, added
cmdsto all thefrom_currsyscalls that I somehow missed last time.I have not touched the tests that use
from_currsysArguably the next step is to simply remove all the
currsyscalls from objects that contain aself.cmdsproperty following the schemevar = self.cmds["!some.thing"]instead offrom_currsys("!some.thing", self.cmds)But this is for next weeks train ride home at 22:00 ...