-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Removing currsys as global parameter #364
Conversation
…l __currsys__ for it to work Added in the logging changs Please enter the commit message for your changes. Lines starting
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## dev_master #364 +/- ##
==============================================
- Coverage 75.12% 72.57% -2.55%
==============================================
Files 58 58
Lines 8005 8052 +47
==============================================
- Hits 6014 5844 -170
- Misses 1991 2208 +217 ☔ View full report in Codecov by Sentry. |
Only the coverage is now failing, so fine to merge, to avoid further complications (discussed with @astronomyk). |
I think this PR broke the IRDB tests. See https://github.com/AstarVienna/irdb/actions/runs/7806706394 and https://github.com/AstarVienna/ScopeSim_Data/actions/runs/7810201424/job/21303256121 👍
It is important to locally run the test suite and notebooks of the irdb before making such large changes. |
|
Simple things like this is now broken: import scopesim as sim
sim.effects.SurfaceList(filename="ELT/LIST_mirrors_ELT.tbl")
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "[...]/ScopeSim/scopesim/effects/surface_list.py", line 24, in __init__
tbl = from_currsys(self.table, self.cmds)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "[...]/ScopeSim/scopesim/utils.py", line 853, in from_currsys
tbl_dict = from_currsys(tbl_dict, cmds)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "[...]/ScopeSim/scopesim/utils.py", line 867, in from_currsys
item[key] = from_currsys(item[key], cmds)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "[...]/ScopeSim/scopesim/utils.py", line 860, in from_currsys
item = np.array([from_currsys(x, cmds) for x in item])
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "[...]/ScopeSim/scopesim/utils.py", line 860, in <listcomp>
item = np.array([from_currsys(x, cmds) for x in item])
^^^^^^^^^^^^^^^^^^^^^
File "[...]/ScopeSim/scopesim/utils.py", line 882, in from_currsys
raise ValueError(f"{item} was not found in rc.__currsys__")
ValueError: !TEL.temperature was not found in rc.__currsys__ @astronomyk do you have an opportunity to look into this? |
@@ -646,7 +649,8 @@ def get_meta_quantity(meta_dict, name, fallback_unit=""): | |||
|
|||
""" | |||
if isinstance(meta_dict[name], str) and meta_dict[name].startswith("!"): | |||
meta_dict[name] = from_currsys(meta_dict[name]) | |||
raise ValueError(f"!-strings should be resolved upstream: " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that we now want to resolve all !-strings upon initialization, but is this really a good idea? There is something to be said to evaluate all !-strings only when they are actually needed.
In particular, would this new scheme mean that it is not possible to easily change the values later? Say the telescope refers to !ATMO.temperature
, which is then resolved upon initialization to, say, 7˘C. Then the user changes !ATMO.temperature
to, say, 8. Would the telescope temperature then stay on 7?
I'm asking, because I'm not sure I understand the new design. (Not sure I understand the old design properly either though.)
Practically though, most files refer to other files (which is the point), but those other files might not yet be loaded. E.g. https://github.com/AstarVienna/irdb/blob/dev_master/ELT/LIST_ELT_combined.tbl refers to !TEL.temperature
. So it is only possible to instantiate SurfaceList(filename="ELT/LIST_mirrors_ELT.tbl")
if the atmosphere has been instantiated.
There could also be cyclic dependencies, because https://github.com/AstarVienna/irdb/blob/dev_master/Armazones/Armazones.yaml refers to !INST.pixel_scale
, but https://github.com/AstarVienna/irdb/blob/dev_master/MICADO/MICADO_IMG_wide.yaml which defines !INST.pixel_scale
refers to !ATMO.altitude
.
Or does this actually work? I'm confused.
So not everything is apparently evaluated early. Apparently !-values are only evaluated early if they occur in a table, but not when they are regular properties (of an optical element) of attributes (of an effect)? E.g. That does not seem right. |
if filename.startswith("!"): | ||
filename = from_currsys(filename) | ||
raise ValueError(f"!-string filename should be resolved upstream: " | ||
f"{filename}") | ||
# filename = from_currsys(filename) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The call to from_currsys
has been removed from find_file()
, so every caller of find_file()
should now do the resolving of !-values themselves. But this has not been done, SpectralSurface.__init__()
in particular does not resolve !-values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it seems quite some work to properly propagate cmd
, e.g. through this traceback:
../ScopeSim/scopesim/effects/surface_list.py:26: in __init__
self.surfaces = rad_utils.make_surface_dict_from_table(tbl)
../ScopeSim/scopesim/optics/radiometry_utils.py:161: in make_surface_dict_from_table
surf_dict[names[ii]] = make_surface_from_row(row, **tbl.meta)
../ScopeSim/scopesim/optics/radiometry_utils.py:169: in make_surface_from_row
surface = SpectralSurface(**kwargs)
../ScopeSim/scopesim/optics/surface.py:50: in __init__
filename = find_file(filename)
OTOH, at first glance it seems that make_surface_dict_from_table()
and make_surface_from_row()
don't do anything surface-specific. Similar functionality should exist for the DetectorArray
for example, so if we generalize those functions, then things might become easier.
Remove the cursed
rc.currsys
object from the global namespace.Rational:
In order to allow multi-core processing, we cannot rely on global properties.
Most
Effect
objects make a call to the utility functionfrom_currsys
in order to resolve an!
-strings.This function tracks down the value(s) that any (chain of) bang-strings refers to.
The current system (
__currsys__
) acts as a member of therc
submodule, and acts essentially as a global variable.This gets in when generating sub-processes to split up the workload of the loops found in the
OpticalTrain.observe
function.An additional side-effect of having a global-esque commands dictionary, is that instances of
OpticalTrain
are not self-contained.As such a user cannot reliably use multiple instances of an
OpticalTrain
in a script of notebook.This prohibits efficient re-use of an already created system, and means ScopeSim fails to achieve the goal of enabling single
Source
objects to be observed with multiple instruments in the same session.Facit: Not User Freindly
Implemention
The work around here contains two parts:
OpticalTrain
object to thefrom_currsys
function, and bypass the call to the global dict stored inrc.__currsys__
.UserCommands
object down through all layers such that eachEffect
object also references the top-levelself.cmds
dict.Goal
Ideally if I have done this correctly, the
rc.__currsys__
dict should not be needed now, and can therefore be deleted.How to test this:
rc.__currsys__
rc.__currsys__
fromfrom_currsys()
set_focus
method fromOpticalTrain
Exceptions
There are occasional calls to
!SIM
keywords. These should be kept in a global level dictionary as they pertain to how scopesim runs. There should be no instrument specific parameters in the!SIM
dict.