Skip to content
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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+62 −0
Open

Validate derrf distribution parameters on startup #9599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions src/ert/config/gen_kw_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,32 @@ def _check_valid_triangular_parameters(prior: PriorDict) -> None:
).set_context(self.name)
)

def _check_valid_derrf_parameters(prior: PriorDict) -> None:
Copy link
Collaborator

@oyvindeide oyvindeide Dec 20, 2024 •
edited
Loading

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

Copy link
Collaborator

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?

key = prior["key"]
dist = prior["function"]
steps, min_, max_, _, width = prior["parameters"].values()
if not (steps >= 1 and steps.is_integer()):
errors.append(
ErrorInfo(
f"NBINS {steps} must be a positive integer larger than 1"
f" for {dist} distributed parameter {key}",
).set_context(self.name)
)
if not (min_ < max_):
errors.append(
ErrorInfo(
f"The minimum {min_} must be less than the maximum {max_}"
f" for {dist} distributed parameter {key}",
).set_context(self.name)
)
if not (width > 0):
errors.append(
ErrorInfo(
f"The width {width} must be greater than 0"
f" for {dist} distributed parameter {key}",
).set_context(self.name)
)

unique_keys = set()
for prior in self.get_priors():
key = prior["key"]
Expand Down
36 changes: 36 additions & 0 deletions tests/ert/unit_tests/config/test_gen_kw_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -681,3 +681,39 @@ def test_validation_triangular_distribution(
ErtConfig.from_file("config.ert")
else:
ErtConfig.from_file("config.ert")


@pytest.mark.parametrize(
"distribution, nbins, min, max, skew, width, error",
[
("DERRF", "10", "-1", "3", "-1", "2", None),
],
)
def test_validation_derrf_distribution(
tmpdir, distribution, nbins, min, max, skew, width, error
):
with tmpdir.as_cwd():
config = dedent(
"""
JOBNAME my_name%d
NUM_REALIZATIONS 1
GEN_KW KW_NAME template.txt kw.txt prior.txt
"""
)
with open("config.ert", "w", encoding="utf-8") as fh:
fh.writelines(config)
with open("template.txt", "w", encoding="utf-8") as fh:
fh.writelines("MY_KEYWORD <MY_KEYWORD>")
with open("prior.txt", "w", encoding="utf-8") as fh:
fh.writelines(
f"MY_KEYWORD {distribution} {nbins} {min} {max} {skew} {width}"
)

if error:
with pytest.raises(
ConfigValidationError,
match=error,
):
ErtConfig.from_file("config.ert")
else:
ErtConfig.from_file("config.ert")
Loading