-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Param mapping #182
base: master
Are you sure you want to change the base?
Param mapping #182
Conversation
Just to clarify (haven't looked at the code yet), the default behavior is if two modules have the same Related, but kind of aside, what happens (in code at |
from pydantic import BaseModel, Field | ||
from typing import Sequence | ||
from pydantic import BaseModel, Field, root_validator | ||
from typing import Sequence, Mapping, Optional | ||
|
||
class Parameter(BaseModel, allow_population_by_field_name = True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In looking at the tests, what are your thoughts on validating that:
init
,min
, andmax
are notnan
(on the fence about this one)init
is in the bounds ofmin
andmax
min
>=max
?
These seem like sane invariants that we should uphold.
Aside, we should add a warning here if a
|
b7482de
to
9875096
Compare
return params | ||
|
||
@pytest.fixture | ||
def multi_model_shared_params2() -> Mapping[str, list[Parameter]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first glance, this overlaps with multi_model_shared_params
s.t. I dont think we need it? Can we just get rid of this fixture?
@@ -11,5 +11,13 @@ class Parameter(BaseModel, allow_population_by_field_name = True): | |||
min: float | |||
max: float | |||
init: float | |||
alias: Optional[str] | |||
|
|||
@root_validator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor improvement that without could result in a misleading error.
@root_validator | |
@root_validator(skip_on_failure=True) |
This should allow a calibration parameter to set an
alias
key, and any params across any models sharing the samealias
name will be treated as the same parameter for the purpose of permutation generation.Note, needs #180 to merge first.