Reconsider effects structure

[teutoburg]

This existed as an empty draft issue (aka "TODO") in the ScopeSim v1.0 project for some time, but now I found some content to put here, i.e. what actually should be discussed here.

This is a summary of a discussion between @hugobuddel, @astronomyk and @teutoburg held elsewhere.

Current state

The various subclasses of Effect are the flesh and guts of ScopeSim. It is thus considered vital to have a solid structure for them going forward. Currently, most effects rely on **kwargs to accept parameters, usually from YAML files. Those parameters are stored in a meta dict, along with a reference to the optical train's configuration in cmds, the latter being optional. Some effect subclasses have a set of required_keys that must be present in the kwargs. Some effects are subclasses of other effects (see ter_curves.py for some good examples), but may have different constructor signatures (i.e. a different set of required_keys) that gets converted for initializing the base class, with its own set of keys.

Bang keys

Various parameters needed by an effect need to reference global (or "regional", i.e. optical train level) configurations, that may change after the effect instance is created, thus a "bang-string-key" is used to lookup those values in cmds. It is not always initially obvious for all effects, exactly which parameters might need lookup and which ones don't.

Two ways forward

Unionist approach

• Keep every parameter in the meta dict, but internally make this a ChainMap, that branches off of the cmds one.
• This would eliminate .cmds as a separate attribute, and rather store everything in meta.
• The lower levels of that ChainMap would be a reference to the global and regional configurations.
• Every attribute stored in this hierarchical meta as a bang-string-key would be automatically resolved by every __getitem__ call. This means from_currsys(self.meta["!SIM.foo"], self.cmds) would become redundant and simply be replaced with self.meta["!SIM.foo"], as the reference is resolved internally.
• It is thus not necessary to know which parameters might need to be resolved at which point in the effect's functionality.
• But: It might be less transparent, which parameters are stored as bang key references, and which are simple values. Might need to add a unresolved option somewhere, which is not ideal either...

Separatist approach

• Essentially keep things more similar to how they currently are, structurally.
• Turn the effects into dataclasses and turn the meta parameters into actual instance attributes.
• This would make it more transparent what attributes are stored (and needed!) by an effect subclass.
• Any bang-string-key would still need to be resolved manually. It's probably useful to add a base class method for this, which can be made more elegant (and robust) than the current from_currsys.
• This would retain transparency for which parameters (attributes) are stored as actual values, and which ones point to global / regional level configurations.
• However, we will still need to manually keep track of which parameters need to be resolved at which point in the logic. This might need a few more guards against rogue bang-strings.

[teutoburg]

Addendum

Like meta, but betta

If we go with the "separatist" approach and dataclasses, we might still retain something emulating the current meta dict simply by using dataclasses.asdict, which converts a dataclass' attributes to a dict.

No more implicit cmds

The "regional config" that is the cmds is now (hopefully) accepted by all effect subclasses. Still needs to be handled in the base class IMO, but that's the easier part. However, while the cmds is now usually passed along when effects are created in an OpticalTrain using effects.effect_utils.make_effect(), this is not at all the case in our testing suite, where effects are usually instantiated directly, and from_currsys falls back to rc.__currsys__. In fact, this is already now a major problem, as it necessitates mocking this environment in our tests. I regularly (again now while experimenting with the "unionist" approach) run into tests that fail when run as part of the (partial) test suite, but pass just fine when I copy-paste that test's code (and fixtures) in an isolated script. This is beyond annoying.

I thus propose phasing out any such "implicit" use of cmds via rc.__currsys__. Most tests for effects should also pass an appropriate (mock) cmds to the effect in question, I suppose with the exception of tests that purposefully test what happens if no cmds is passed. In fact, this might be a case for #367...

[hugobuddel]

Another idea to harmonize both approaches by having a flag or something like that to indicate whether the bang-strings should be resolved or not.

It should really not be necessary in the 'separatist' approach to track which parameters need to be resolved, because effectively, all of them need to be resolved, because all of them can be a bang-string. So any call to self.abc or self.meta['abc'] would need to be wrapped in (what is now) from_currsys(), unless explicitly desired to get the bang-string.

Having to wrap everything in from_currsys() is of course pretty annoying, and prone to mistakes. E.g. we forget the from_currsys() in one place where we don't usually use a bang-string. Then some user decides to use a bang-string there, and our code fails!

What we maybe could do is have some flag to indicate whether we want to have the lookup done, and set this flag in the functions of the effect. E.g. through a decorator like @resolve_bang_strings that we decorate all functions with (maybe automatically?). Then within the functions, all bang-strings are resolved.

I think we can follow such a scheme with both the Unionist and Separatist approach. For example by having the flag in the ChainMap, or by overloading __getattr__() and having the flag there.

[hugobuddel]

A question about the unionist approach: if the user does myeffect.meta.keys(), what should the result be?

I think myeffect.meta should only have the attributes that the user can set specifically for that effect. The effect might need other parameters, e.g. !SIM.somesetting, but I'm not sure it is useful to expose those in myeffect.meta. Most parameters in the chainmap would not be relevant at all I believe, and then the perceived benefit of "meta is clearer for our users than attributes" kinda flies out of the window I think.

[teutoburg]

One further point I forgot: Part of the original idea for the chain map was to have the (default) params that many effects have as the second layer (from the top), so that effectively anything (optional) not passed as a kwarg would be found there. Now with dataclasses, it's of course trivial to have default values for attributes, which fulfills the same purpose. However, a key difference is that default values for attributes are simply overwritten when that attribute is supplied, whereas the chain map still has the default value accessible in the second layer. Thus, it would be easy to "restore to factory settings" by simply deleting that key from the chain map (as this only affects the top layer). But I'm now questioning if that's at all practicality relevant! If not, then that's one argument less for the unionists.

[hugobuddel]

Yes I think that having all 'settings' in one place is a large value proposition of the unionist approach. The chainmap solution is rather beautiful, and worth having.

Actually, I don't think there is a necessarily a discrepancy between the "chainmap" and "dataclass" approach (hence me renaming them from "unionist" and "separatist").

For me the appealing part of the dataclass approach is that the "kwargs" of an effect become Python attributes, which I like for various reasons. That benefit of dataclasses is kinda independent of the benefit of the chainmap, because the attributes can still (also) be in the chainmap.

Perhaps we could get it to work like this:

• Effects are dataclasses (or something similar we create), so the kwargs are (automatically) attributes (or properties, see below).
• The kwargs also are a 4th (or whatever) layer in the chainmap in meta.
• Accessing through meta will, by default, get the unresolved attribute.
• Accessing as an attribute will retrieve the value recursively through meta (so maybe they should be properties that do this lookup, instead of simple attributes).

The reason to have the defaults as in the last two bullets (and not the other way around) is because that makes typing easier:

• The default type of meta will be Mapping[Str, Any], because we can't really get it more precise anyway.
• The type of the attributes will actually be the true type instead of Union[the_actual_type, Str], which would be needed to accomodate the !-string values.

I'm now thinking that such a attribute+chainmap approach could answer the to-resolve-or-not-to-resolve dilemma quite naturally: resolve when accessing as an attribute, do not resolve when accessing through the chainmap, unless explicitly asked for.

A related thought is that this approach would fail when an apply_to() method accesses a value from meta (/currently cmds) that is not actually an attribute, because then we would need to explicitly turn resolving on. But this would perhaps highlight a problem: apparently the Effect needs values that are not properly declared in its class definition. Any value it need should (perhaps indirectly) be accessed through its attributes, which could then in turn be a bang-string.

E.g. if some effects needs a temperature, but it is realistically always the temperature of the instrument. Then it should still have this temperature in its kwargs, with a default to !INST.temperature. So the effect should only refer to self.temperature, which would look up self.meta['temperature'] recursively. It could be considered bad practice to access self.meta["!INST.temperature"] directly.

I'm not saying this idea is necessarily the best approach, but I do feel that we are getting closer to consensus of what the best approach would be.

[hugobuddel]

Something like

from dataclasses import dataclass, field


@dataclass
class Effect:
    meta = None
    
    def __post_init__(self):
        # Populate self.meta.
        setattr(self, "meta", {
            k: getattr(self, k)
            for k in self.__dataclass_fields__
        })
    
    def __getattribute__(self, k):
        # Get self.meta in the usual way.
        if k in {"meta"}:
            return object.__getattribute__(self, k)
        # Resolve values in self.meta through the chainmap.
        if self.meta and k in self.meta:
            return self.meta[k]
        # Return any other value in the normal way.
        return  object.__getattribute__(self, k)

    def __setattr__(self, k, v):
        if k in {"meta"}:
            return object.__setattr__(self, k, v)
        if self.meta and k in self.meta:
            self.meta[k] = v
        return object.__setattr__(self, k, v)
        

@dataclass
class MyEffectA(Effect):
    a: int = field(kw_only=True)
    b: int = field(kw_only=True, default=5)

@dataclass
class MyEffectB(MyEffectA):
    c: int = field(kw_only=True)



myeff = MyEffectB(a=4, c=3)
print(myeff.a)
print(myeff.meta)

myeff.meta['a'] = 9
print(myeff.a)
print(myeff.meta)

myeff.a = 7
print(myeff.a)
print(myeff.meta)

Seems to work. Output:

4
{'a': 4, 'b': 5, 'c': 3}
9
{'a': 9, 'b': 5, 'c': 3}
7
{'a': 7, 'b': 5, 'c': 3}
``

[hugobuddel]

I couldn't resist to do it with an actual NestedChainMap. I monkey-patched the class so it resolves conditionally; only when self.resolve == True. This is a big kludge, it would be better e.g. have __getitem__() never resolve and have a get_resolved() method. Or whatever, we can think of something better.

from dataclasses import dataclass, field
from scopesim import UserCommands
from astar_utils import NestedMapping, RecursiveNestedMapping, NestedChainMap
from collections import abc, ChainMap

# Monkey patch NestedChainMap to make resolving optional. Kinda hacky.
NestedChainMap.old__getitem__ = NestedChainMap.__getitem__
NestedChainMap.resolve = False
NestedChainMap.__getitem__ = lambda self, key: self.old__getitem__(key) if self.resolve else ChainMap.__getitem__(self, key)

@dataclass
class Effect:
    meta = None
    cmds: UserCommands = None
    
    def __post_init__(self):
        # Populate self.meta.
        themetadict = {
                k: getattr(self, k)
                for k in self.__dataclass_fields__
                if k not in {"cmds", "title"}
            }
        themeta = NestedChainMap(
            RecursiveNestedMapping(title="Meta"),
            *[self.cmds],
        )
        themeta.update(themetadict)
        setattr(self, "meta", themeta)
    
    def __getattribute__(self, k):
        # Get self.meta and self.cmds in the usual way.
        if k in Effect.__dict__:
            return object.__getattribute__(self, k)
        # Resolve values in self.meta through the chainmap.
        if self.meta and k in self.meta:
            # Hackish way to only resolve when an attribute is used.
            self.meta.resolve = True
            v = self.meta[k]
            self.meta.resolve = False
            return v
        # Return any other value in the normal way.
        return  object.__getattribute__(self, k)

    def __setattr__(self, k, v):
        if k in {"meta"}:
            return object.__setattr__(self, k, v)
        if self.meta and k in self.meta:
            self.meta[k] = v
        return object.__setattr__(self, k, v)
        

@dataclass
class MyEffectA(Effect):
    a: int = field(kw_only=True)
    temp: int = field(kw_only=True, default="!TEL.temperature")

@dataclass
class MyEffectB(MyEffectA):
    c: int = field(kw_only=True)

cmds = UserCommands(packages=["test_package"],
                        yamls=["test_telescope.yaml",
                               {"alias": "ATMO",
                                "properties": {"pwv": 9001}}],
                        properties={"!ATMO.pwv": 8999})


myeff = MyEffectB(a=4, c=3, cmds=cmds)
print(f'{myeff.temp=}')
print(f'{myeff.meta["temp"]=}')
print(f'{myeff.meta["!TEL.temperature"]=}')
print()

cmds["!TEL.temperature"] = 888
print(f'{myeff.temp=}')
print(f'{myeff.meta["temp"]=}')
print(f'{myeff.meta["!TEL.temperature"]=}')
print()

myeff.temp = -20
print(f'{myeff.temp=}')
print(f'{myeff.meta["temp"]=}')
print(f'{myeff.meta["!TEL.temperature"]=}')
print()

Prints out:

myeff.temp=9001
myeff.meta["temp"]='!TEL.temperature'
myeff.meta["!TEL.temperature"]=9001

myeff.temp=888
myeff.meta["temp"]='!TEL.temperature'
myeff.meta["!TEL.temperature"]=888

myeff.temp=-20
myeff.meta["temp"]=-20
myeff.meta["!TEL.temperature"]=888

which is exactly what we expect.

Good idea? Or bad idea?

[teutoburg]

@hugobuddel I'm slowly getting back into this (after almost exactly 7 months 😛) because I think it might make resolving the whole AutoExposure / SummedExposure DIT, NDIT, exptime thingy (which is currently a major road block for me) a lot easier, as well as all kinds of other things. A few comments:

The proposed solution used kw_only=True for every field of any derived effect class. The @dataclass decorator also has the same optional parameter which is then applied to all fields, so I'd say we just use that. I don't think there would be any case where we don't want a parameter to be keyword-only (except in the base Effect class maybe, as per your example above)?

If we allow (not doing so would defeat the whole idea here) parameters to be (bang) strings, regardless of the type that the parameter would otherwise have, we should also reflect this in the typing of the parameter. I.e., using int and then setting the default to a bang string will still work (because typing is optional in Python as it should be), but is rather dirty and would cause some headache if we ever decide to use mypy et al. (which I already try to do for new code). Maybe there's a clever typey (typingy? typy?) way to only allow actual bang-strings and not just any old string? I.e. make bang-strings a custom type? That would also eliminate the need for astar_utils.is_bangkey(), which could then just be replaced by isinstance(s, BangString). Given that most of these parameters are numeric (int or float or any of the numpy types), probably the most correct way would be a custom type that allows those plus bang-strings (if a parameter is actually a string, it's much easier, just use str and be done with it). But I don't know enough about typing to tell if something like that is possible and stable. In the meantime, we might just use Any for the parameter types.

In terms of how to control resolving NestedChainMap values: How about using the already existing (and documented!) syntax of trailing ! in a key? This is actually currently implemented in Effect.__getitem__ already! So instead of resolve=True we might just do [f"{k}!"]. Although then I'm unsure if this functionality should be in NestedChainMap.__getitem__ and/or (stay) in Effect.__getitem__? But that should be fairly easy to sort out...

Good idea? Or bad idea?

I couldn't come up with a better one in 7 months 😄

[hugobuddel]

Maybe just do

class BangString(str):
    ...

and take it from there?

But we should allow the user to just specify a string, not require them instantiate a BangString. So we still need all that old logic, especially when the value can be a normal string too.

The trailing ! seems like a quite elegant (or totally confusing?) way to resolve them, so let's just use that. I didn't know that existed either!

I was thinking maybe pydantic could help us here, but I think that has other usecases.