Improve serialization of functional profiles #276
Conversation
for more information, see https://pre-commit.ci
… into serialize-functions
for more information, see https://pre-commit.ci
bmaranville
left a comment
bmaranville
left a comment
There was a problem hiding this comment.
This looks fine to me - I tried it out on a couple models that use FunctionalProfile, and it seems to work. Maybe @acaruana2009 and @hoogerheide might try it to make sure it still does what it is supposed to with those models?
I don't feel as comfortable with the **kwargs interface for setting parameters, or the __setattr__ trap (too many magical and unexpected interfaces), but that is a matter of taste and there is definitely convenience added for those that are willing to learn the mental model.
| else: | ||
| super().__setattr__(key, value) | ||
|
|
||
| def __getattr__(self, key): |
There was a problem hiding this comment.
Wait... won't this block access to all the attributes that aren't in pars?
bmaranville
left a comment
bmaranville
left a comment
There was a problem hiding this comment.
I am worried about the __getattr__ override in FunctionalProfile... I don't see how you can access the regular attributes of the class with this in place.
|
note that |
bmaranville
left a comment
bmaranville
left a comment
There was a problem hiding this comment.
I had forgotten the subtlety of when __getattr__ is called... and the attribute-based access to pars is preserving the existing API - I have no further objections to this.
Redo #219 using the generic function serialization in bumps.
Requires bumps serialize-functions branch for the tests to pass.