REF: templated comparisons in constraints tool - #189
Conversation
…ce shared comparison configurations held by ConstraintsConfig
19acab8 to
7310709
Compare
| crs, reg = super().run(obs_map, saved_obs_map, comparison_map, group) | ||
| if not isinstance(self.comparison, IsLess): | ||
| raise TypeError( | ||
| f"Referenced comparison ({self.comparison}) is of" |
There was a problem hiding this comment.
I make this mistake all the time so I pick up on it more quickly nowadays: missing a trailing space (message will be is ofincorrect type after concatenation)
| if isinstance(self.comparison, str): | ||
| if self.comparison not in comparison_map: | ||
| raise ValueError(f"Referenced comparison ({self.comparison}) not defined") | ||
| self.comparison = cast(CompT, comparison_map[self.comparison]) |
There was a problem hiding this comment.
Dereferencing the comparison is necessary, but if you update self.comparison you can no longer serialize back to the input constraints.yaml correctly.
There was a problem hiding this comment.
A good point. Perhaps it's worth stashing the object in a separate, non-serialized attribute
There was a problem hiding this comment.
There'll be more to do in the followup to ensure roundtrips are reasonable (or at least faithful to the way configs are written currently)
There was a problem hiding this comment.
I promise to DRY this out in a followup 😥
| comparison_map : dict[str, AnyComparison] | ||
| mapping string -> shared comparisons | ||
| group : str | None | ||
| _description_ |
There was a problem hiding this comment.
I clearly had ambitions to complete the docstrings here but forgot about them...
| ) | ||
| comparisons: dict[str, AnyComparison] = Field( | ||
| default_factory=dict, | ||
| description="Mapping from unimque comparison identifier to reusable comparison settings", |
There was a problem hiding this comment.
| description="Mapping from unimque comparison identifier to reusable comparison settings", | |
| description="Mapping from unique comparison identifier to reusable comparison settings", |
A unique spelling of unique, indeed 😉
| self, | ||
| obs_map: dict[Observable, Observation], | ||
| compare_map: dict[Observable, Observation] | None, | ||
| saved_obs_map: dict[Observable, Observation] | None, |
There was a problem hiding this comment.
So saved observations are like the baseline to compare against, obs_map is the current set of observations that we're going to compare using comparison_map comparisons... right?
Baseline, reference, or perhaps "expected" are all more clear (personally) from a testing/validation standpoint.
Naming is also a bit unfortunate with x_map as it doesn't tell you anything about the key or value. x_to_y is so much nicer to work with from an API/user standpoint, but observable_to_observation is too long, and both words have identical prefixes so shortening them is complicated.
OK, that's all the nitpicking I'll do about the naming. Maybe we can revisit the incredible number of classes and naming down the line.
There was a problem hiding this comment.
I had similar troubles regarding naming here. The saved observations here are the observations generated from the lattice files, I think "expected_obs_map" could work here.
I do intend to take a second pass at the submodule to try and simplify things, but I can make that change along with compare_map -> common_comparisons_map
…eserve serialization roundtrip post-.run()
2 participants
Description
Context
The current schema resulted in lots of comparison duplication, and we'd like to DRY this out a bit.