-
Notifications
You must be signed in to change notification settings - Fork 109
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
Validate derrf distribution parameters on startup #9599
Conversation
CodSpeed Performance ReportMerging #9599 will not alter performanceComparing Summary
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9599 +/- ##
==========================================
- Coverage 91.74% 91.71% -0.03%
==========================================
Files 430 430
Lines 26639 26661 +22
==========================================
+ Hits 24439 24452 +13
- Misses 2200 2209 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -224,6 +224,32 @@ def _check_valid_triangular_parameters(prior: PriorDict) -> None: | |||
).set_context(self.name) | |||
) | |||
|
|||
def _check_valid_derrf_parameters(prior: PriorDict) -> None: |
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.
It might be outside the scope for this PR, but did you consider converting this from a function to a dataclass? Something like:
@pydantic.dataclass
class DERRF:
name: str
steps: PositiveInt
min: float
max: float
skewness: float
width: confloat(gt=0)
@model_validator(mode="after")
def validate_min_max(self) -> Self:
if not self.max > self.min
raise ValueError(f"Max ({self.max}) must be larger than max ({self.max})
return self
def transform(self, x: float) -> float:
"""
Bin the result of `trans_errf` with `min=0` and `max=1` to closest of `nbins`
linearly spaced values on [0,1]. Finally map [0,1] to [min, max].
"""
q_values = np.linspace(start=0, stop=1, num=self.steps)
q_checks = np.linspace(start=0, stop=1, num=self.steps + 1)[1:]
y = TransformFunction.trans_errf(x, [0, 1, self.skewness, self.width])
bin_index = np.digitize(y, q_checks, right=True)
y_binned = q_values[bin_index]
result = self.min + y_binned * (self.max - self.min)
if result > self.max or result < self.min:
warnings.warn(
"trans_derff suffered from catastrophic loss of precision, clamping to min,max",
stacklevel=1,
)
return np.clip(result, self.min, self.max)
if np.isnan(result):
raise ValueError(
"trans_derrf returns nan, check that input arguments are reasonable"
)
return result
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.
I did not, but will look into it.
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.
I guess this will require a storage migration?
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.
Yes, I guess it will, though it would be possible to work around it for now, and only migrate once we do something like this for all parameter types. If that is the direction you are heading with rewriting parameter configs?
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.
I suggest we merge this fix for now and then you can do this conversion later.
5655414
to
711f8e7
Compare
711f8e7
to
c3de39f
Compare
c3de39f
to
b51d234
Compare
Issue
Resolves #9523
git rebase -i main --exec 'pytest tests/ert/unit_tests -n logical -m "not integration_test"'
)When applicable