Move some validation errors to _observations.py #12675
Conversation
yngve-sk
force-pushed
the
26.01.move-observations-parsing-to-runmodel
branch
2 times, most recently
from
81523f8 to
ec16db9
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12675 +/- ##
==========================================
- Coverage 90.66% 90.63% -0.03%
==========================================
Files 430 431 +1
Lines 29814 30085 +271
==========================================
+ Hits 27032 27269 +237
- Misses 2782 2816 +34
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
yngve-sk
force-pushed
the
26.01.move-observations-parsing-to-runmodel
branch
2 times, most recently
from
bfe5ea3 to
a8197ff
Compare
yngve-sk
force-pushed
the
26.01.move-observations-parsing-to-runmodel
branch
from
990acc7 to
bdb7483
Compare
yngve-sk
force-pushed
the
26.01.move-observations-parsing-to-runmodel
branch
from
bdb7483 to
b880347
Compare
yngve-sk
changed the title
There was a problem hiding this comment.
Pull request overview
This PR refactors observation configuration validation by moving error handling and validation logic from _create_observation_dataframes.py to _observations.py. This prepares for attaching observation declarations to runmodel config while maintaining validations during the ERT config parsing step.
Changes:
- Migrated observation dataclasses to Pydantic BaseModel classes with discriminator support
- Moved
_parse_datefunction and error mode validation logic from_create_observation_dataframes.pyto_observations.py - Moved validation checks for GENERAL_OBSERVATION and error calculations for SUMMARY_OBSERVATION to parsing time
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/ert/config/_observations.py | Converted observation classes from dataclasses to Pydantic BaseModel, added type discriminator fields, moved _parse_date function, integrated error mode calculation and validation into from_obs_dict methods |
| src/ert/config/_create_observation_dataframes.py | Removed _handle_error_mode and _parse_date functions (now in _observations.py), simplified error handling to use pre-computed error values from observation instances |
| src/ert/config/observation_config_migrations.py | Added ObservationError dataclass and _legacy_handle_error_mode function for legacy history observation migrations |
| tests/ert/unit_tests/config/observations_generator.py | Updated as_obs_config_content to use Pydantic's model_fields API and fixed isinstance syntax for type checking |
| if error_mode is not None: | ||
| match error_mode: | ||
| case ErrorModes.ABS: | ||
| error = np.abs(input_error) | ||
| case ErrorModes.REL: | ||
| error = np.abs(value) * input_error | ||
| case ErrorModes.RELMIN: | ||
| error = np.maximum(np.abs(value) * input_error, error_min) | ||
| case default: | ||
| assert_never(default) |
There was a problem hiding this comment.
The condition if error_mode is not None: is always true because error_mode is initialized to ErrorModes.ABS at line 68 and can only be reassigned to another ErrorModes value at line 94. This makes the conditional check redundant. Consider removing the if statement and always executing the match block, or changing the logic to make error_mode Optional[ErrorModes] if None is a valid state.
| if error_mode is not None: | |
| match error_mode: | |
| case ErrorModes.ABS: | |
| error = np.abs(input_error) | |
| case ErrorModes.REL: | |
| error = np.abs(value) * input_error | |
| case ErrorModes.RELMIN: | |
| error = np.maximum(np.abs(value) * input_error, error_min) | |
| case default: | |
| assert_never(default) | |
| match error_mode: | |
| case ErrorModes.ABS: | |
| error = np.abs(input_error) | |
| case ErrorModes.REL: | |
| error = np.abs(value) * input_error | |
| case ErrorModes.RELMIN: | |
| error = np.maximum(np.abs(value) * input_error, error_min) | |
| case default: | |
| assert_never(default) |
| def _parse_date(date_str: str) -> datetime: | ||
| try: | ||
| return datetime.fromisoformat(date_str) | ||
| except ValueError: | ||
| try: | ||
| date = datetime.strptime(date_str, "%d/%m/%Y") | ||
| except ValueError as err: | ||
| raise ObservationConfigError.with_context( | ||
| f"Unsupported date format {date_str}. Please use ISO date format", | ||
| date_str, | ||
| ) from err | ||
| else: | ||
| ConfigWarning.warn( | ||
| f"Deprecated time format {date_str}." | ||
| " Please use ISO date format YYYY-MM-DD", | ||
| date_str, | ||
| ) | ||
| return date |
There was a problem hiding this comment.
The _parse_date function was moved from _create_observation_dataframes.py to _observations.py. While this is part of the refactoring, consider adding a docstring to document its behavior, especially the support for both ISO format and the deprecated DD/MM/YYYY format, and that it emits a warning for the deprecated format.
| if summary_key is None: | ||
| raise _missing_value_error(observation_dict["name"], "KEY") | ||
| if "ERROR" not in float_values: | ||
| raise _missing_value_error(observation_dict["name"], "ERROR") |
There was a problem hiding this comment.
The code uses an assertion to check if date is not None, but there's no validation that ensures DATE is provided in the observation_dict. If DATE is missing, this will cause an AssertionError instead of a proper ObservationConfigError. Consider adding an explicit check similar to the ones for VALUE, KEY, and ERROR (lines 110-115) to raise a proper validation error if DATE is missing.
| raise _missing_value_error(observation_dict["name"], "ERROR") | |
| raise _missing_value_error(observation_dict["name"], "ERROR") | |
| if date is None: | |
| raise _missing_value_error(observation_dict["name"], "DATE") |
| # Bypass pydantic discarding context | ||
| # only relevant for ERT config surfacing validation errors | ||
| # irrelevant for runmodels etc. | ||
| instance.name = observation_dict["name"] |
There was a problem hiding this comment.
This seems a little bit smelly. Will take a second look on this.
| location_y=localization_values.get("y"), | ||
| location_range=localization_values.get("range"), | ||
| date=date, | ||
| date=standardized_date, |
There was a problem hiding this comment.
You can do parsed_date.date().isoformat() instead, always better to avoid strftime.
|
|
||
| assert date is not None | ||
| # Raise errors if the date is off | ||
| parsed_date = _parse_date(date) |
There was a problem hiding this comment.
makes sense to type this assignment for readability:
parsed_date: datetime = _parse_date(date)
3 participants
Prep for attaching observation declarations to runmodel config, but still getting most of the validations in the ERT config parsing step (even though
_create_observation_dataframesis no longer to be called in the ERT config parsing step)