Add more bang-string-key functionality

[teutoburg]

Modifications

NestedMapping

• Modify .__getitem__() to return another instance of NestedMapping (or any subclass), if the about-to-be-returned value is another nested dict. This effectively enables the further use of the bang-string-key syntax on extracted sub-dicts.
• Minimal refactoring also to .__setitem__() and .__delitem__(), without any functional changes.
• Addition of a _repr_pretty_() method for nice output in an IPython console.

Other

We were now importing so many classes from collections.abc, and also now ChainMap directly from collections, that I decided to just do from collections import abc, ChainMap and add the abc. to all those base classes here. No functional changes here.

New classes and functions

RecursiveNestedMapping

A subclass of the existing NestedMapping, this simply modifies the .__getitem__(key) method such that if a returned value is itself a valid bang-string, it will be used to (recursively) look up the value associated with it. If that bang-string is not a key in the mapping, it will be returned as-is, i.e. as an unresolved bang-string. In the case of a "chain" of bang-strings each pointing to the next one (e.g. "!FOO.bar": "!FOO.baz" ➡️ "!FOO.baz": "!BAR.foo" ➡️ "!BAR.foo": 42), that chain will be recursively resolved until either a non-bang-string value (either a str without a leading "!" or not a str at all) is found, or a bang-string that is not a valid key in the mapping, in which case the final, unresolved string will be returned.

Additionally, an alternative constructor is provided in the .from_maps(maps, key) class method. This is useful for, and currently only used by, the NestedChainMap (see below), yielding numbered instances for each map in maps that contains the specified key.

"But @teutoburg!", I hear you yell, "What about infinite recursion??"

Fear not! In the case that someone was silly enough to create a "loop" of bang-string keys pointing back to the first one (e.g. "!FOO.bar": "!FOO.baz" ➡️ "!FOO.baz": "!BAR.foo" ➡️ "!BAR.foo": "!FOO.bar" ➡️ "!FOO.bar": "!FOO.baz" ➡️ ♾️), Python's builtin recursion limit will kick in virtually instantly and yeet a standard RecursionError.

NestedChainMap

A subclass of collections.ChainMap using using RecursiveNestedMapping. It only overrides the .__getitem__(key) method to allow for both recursive bang-string-keys pointing across the individual mappings and to "collect" sub-mappings from the same bang-key in multiple mappings into a new NestedChainMap. Also overrides .__str__() and provides a ._repr_pretty_() for nice output to an IPython console.

In the absence of any nested mappings or bang-string-keys, this will work like the base class collections.ChainMap, meaning an ordinary dict can also be used as one of the individual mapping "levels". For the dreaded case of infinite loops of bang-string-keys pointing to each other, the behavior is identical to this case in a RecursiveNestedMapping, also across multiple mappings.

is_bangkey(key)

Convenience function, return True if key is a) a str and b) starts with a "!", False otherwise. Intended as a shorthand to avoid having to spell out the isinstance and .startswith all the time...

is_nested_mapping(mapping)

Convenience function, return True if a) mapping is Mapping and b) at least one value in mapping is itself another Mapping, False otherwise. Used to determine if it makes sense to wrap mapping in a NestedMapping.

Rejected alternatives

Building the recursive function into the existing NestedMapping class

While easier, the recursive functionality is only needed in some cases (e.g. ScopeSim) and not in others (e.g. SpeXtra). It is, IMO, clearly a distinct functionality from just providing a short-hand syntax for accessing nested dicts, like NestedMapping does, so should be kept separately.

Putting either or both of the new classes in ScopeSim

While currently only used in ScopeSim, it would somewhat defeat the purpose of outsourcing things here to do that. At least for the RecursiveNestedMapping, which directly builds on NestedMapping. And since NestedChainMap also used RecursiveNestedMapping internally, I think it makes most sense to keep both classes here. This might have the added benefit of maybe reusing them somewhere else in the future.

RecursiveNestedChainMap / NestedRecursiveChainMap / RecursiveChainMap

YAGNI, at least for now...

Maybe in the future...

• I'm currently also in the process of shuffling the "alias-properties" functionality over to ScopeSim's responsibility, so once that's done and tested, I'll remove it here in another PR (or probably have it still working but add a DepcrecationWarning).
• Add a way to retrieve a recursive bang-string in a value from a RecursiveNestedMapping. The default will now be to always (attempt to) resolve such cases, but there might be situations where it is required to extract "where it's pointing to", rather than the final resulting value. Currently, the only way to do this is to access the internal mapping via the old .dic attribute. Although the string representation (and also the __repr__ for that matter) do return everything UNresolved, so inspection is already possible that way.
• Since the module now contains more than just NestedMapping and a helper function, I'm considering renaming it to something like bang_maps. Other (less innuendo-prone) suggestions are welcome...

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3905bde) 99.05% compared to head (ee44f80) 99.16%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #33      +/-   ##
==========================================
+ Coverage   99.05%   99.16%   +0.11%     
==========================================
  Files           5        5              
  Lines         316      361      +45     
==========================================
+ Hits          313      358      +45     
  Misses          3        3              

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

[teutoburg]

I realize I should probably include this long PR description somewhere in this package's documentation...

[hugobuddel]

I realize I should probably include this long PR description somewhere in this package's documentation...

Perhaps we should pay more attention to that indeed. It would also be good to copy it in the commit message. Now we are kinda vendor locking ourselves into github.

I don't really understand why a ChainMap would be useful here. As in, you probably have some use-case for these new classes in mind, so please explain those in the PR. Wouldn't using one mapping and updating that suffice?

It also seems a bit heavy for what we use these classes for. But it is not really that complicated, and well-tested, so it's fine.

About the general design, I do see a possible complication when adding #-strings into the mix. For example, see this line in https://github.com/AstarVienna/ScopeSim/blob/dev_master/docs/source/5_liners/bang_strings.ipynb:

opt["#exposure_action.ndit"]

which returns

'!OBS.ndit'

Optimally we would like to resolve that too, and I'm not sure how that neatly fits into the current design.

In particular, it is now necessary to refer to !OBS.ndit in the header keywords yaml files, while it would be more natural to refer to opt["#exposure_action.ndit"], because the latter would get the correct ndit value irrespective of how it has been set.

[hugobuddel]

Hope I didn't cause nightmares with my hash-string comment! This PR is an improvement though.

[teutoburg]

Hope I didn't cause nightmares with my hash-string comment!

TBH, I kinda ignored the hash-strings until now. Mostly because I never really encountered them "in the wild" while working on/with ScopeSim. In hindsight, I probably should have spent more attention to them. But we can always work them in at a later stage I guess.

[teutoburg]

Perhaps we should pay more attention to that indeed.

At least some parts of this PR's text are actually more or less copy-pasted from the class/function docstrings. But yeah, this could be more exhaustive...

[teutoburg]

I don't really understand why a ChainMap would be useful here. As in, you probably have some use-case for these new classes in mind, so please explain those in the PR. Wouldn't using one mapping and updating that suffice?

This all is in preparation to a larger change in ScopeSim, where I'll be using the ChainMap to replace the rc.__config__ / rc.__currsys__ / cmds kerfuffle. In that case it does make a lot of sense to me, and it already passes in my local branch. (That is, it passes the ScopeSim test suite, I've yet to test it against IRDB etc., as I'm trying to learn from the mistakes of others.)

So the full explanation will come in ScopeSim, once all that's ready. TLDR short summary: the ChainMap allows to keep different levels of configurations separate, the whole point is to not update the underlying dict(s), which is/are included "by-ref".

[hugobuddel]

Hope I didn't cause nightmares with my hash-string comment!

TBH, I kinda ignored the hash-strings until now. Mostly because I never really encountered them "in the wild" while working on/with ScopeSim. In hindsight, I probably should have spent more attention to them. But we can always work them in at a later stage I guess.

I think my immediate problem is already solved: opt["#exposure_action.ndit!"] (with exclamation mark at the end) does resolve the bang-string, which I either didn't know or forgot. So we can use that for the header keywords.

This does mean that we have !-strings, #-strings, and #!-strings. So there is that.

It does seem (currently) that once you reach !-strings while recursing, you will not encounter a #-string. So at the moment these two are somewhat isolated.

[hugobuddel]

the ChainMap allows to keep different levels of configurations separate, the whole point is to not update the underlying dict(s), which is/are included "by-ref".

Thanks. I guessed something like that. Sounds good.

[teutoburg]

This does mean that we have !-strings, #-strings, and #!-strings. So there is that.

This does (as you probably know already) confirm to what we state in the docs, see the last paragraph of https://scopesim.readthedocs.io/en/latest/5_liners/bang_strings.html##-strings-are-for-accessing-Effect-object-parameters

It does seem (currently) that once you reach !-strings while recursing, you will not encounter a #-string. So at the moment these two are somewhat isolated.

Let's just hope for the best then 🙂