Maintenance of the Basis class #493
Conversation
There was a problem hiding this comment.
Thanks for this clean up @rileyjmurray, it's super helpful. I've made a few comments and will approve once you take a look at them, but there are no major changes being requested.
| @@ -1782,283 +1760,3 @@ def create_simple_equivalent(self, builtin_basis_name=None): | |||
| if all([c.name == first_comp_name for c in self.component_bases]): | |||
| builtin_basis_name = first_comp_name # if all components have the same name | |||
| return BuiltinBasis(builtin_basis_name, self.elsize, sparse=self.sparse) | |||
|
|
|||
|
|
|||
| class EmbeddedBasis(LazyBasis): | |||
There was a problem hiding this comment.
No action items, just a personal note that we are removing this so that I can include it in my spring cleaning section of the release notes.
| # ^ The original implementation would return this value under two conditions. | ||
| # Either arg was None, or isinstance(arg,(tuple,list,ndarray)) and len(arg) == 0. | ||
| # We're just slightly relaxing the type requirement by using this check instead. | ||
|
|
||
| # At this point, original behavior would check that arg is a tuple, list, or ndarray. | ||
| # Instead, we'll just require that arg[0] is well-defined. This is enough to discern | ||
| # between the two cases we can still support. |
There was a problem hiding this comment.
These comments are helpful while doing review, but will be increasingly less useful as time goes on and we forget what the "original" was doing. Thoughts on removing these comments (at minimum) or replacing them with something that is more self-contained and explain what the check is looking for and why we cast specific ways in that case (stretch)?
This PR makes changes to the Basis class to help with my leakage PR (#410).
Specifics, at time of writing:
About EmbeddedBasis ...
It turns out that the EmbeddedBasis class isn't used anywhere in pyGSTi. (There are a few places that import it, but they still never use it.) I discovered this when I got an error trying to access
eb.elementsfor an EmbeddedBasiseb. That trigged evaluation of EmbeddedBasis._lazy_build_elements, which tried to call an undefined function. See here for a repo-wide search for that function.I'd like to use this PR as an opportunity to either (a) make EmbeddedBasis work, possibly with new semantics, or (b) remove EmbeddedBasis altogether.After getting @enielse's blessing I've decided to remove EmbeddedBasis.