Migrate timemap/refcase dependent observations#12629
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a migration tool to convert deprecated HISTORY_OBSERVATION to SUMMARY_OBSERVATION and remove dependencies on TIME_MAP and REFCASE from observation configurations. The tool aims to help users migrate away from deprecated features as part of issue #12619.
Changes:
- Adds new
ert_config_migrations.pymodule with observation migration logic - Adds
convert_observationsCLI command to__main__.py
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 18 comments.
| File | Description |
|---|---|
| src/ert/config/ert_config_migrations.py | New module implementing observation migration logic including text editing, observation conversion, and configuration parsing |
| src/ert/main.py | Adds CLI subcommand convert_observations to invoke the migration tool |
cd74709 to
d486212
Compare
b71c665 to
5acb06d
Compare
5ec12c5 to
2a46e17
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12629 +/- ##
==========================================
- Coverage 90.66% 90.63% -0.04%
==========================================
Files 430 431 +1
Lines 29814 30067 +253
==========================================
+ Hits 27032 27252 +220
- Misses 2782 2815 +33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
900c0e0 to
8bdce57
Compare
8bdce57 to
e81be7b
Compare
Think that should be covered by tests, and it should be removed from |
195008b to
38f4b1f
Compare
d9c193b to
a4b65dc
Compare
a4b65dc to
38f4b1f
Compare
oyvindeide
left a comment
There was a problem hiding this comment.
Nice! Just some minor comments
| { | ||
| VALUE = 0.9; | ||
| DATE = 2014-09-10; | ||
| KEY = FOPR; |
There was a problem hiding this comment.
Minor comment, but the KEY should be GOPR:FIELD and the name of the observation something else.
| setattr( | ||
| date_dict, str(key).lower(), validate_positive_float(value, key) | ||
| case "DAYS" | "HOURS" | "RESTART": | ||
| raise ObservationConfigError.with_context( |
There was a problem hiding this comment.
How much work is it to raise this as a specific error that we then catch higher up to add context, for example SummaryObservationError, the reason being that we dont need the context of the cli tool in this part of the code, that can live higher in the stack. Should still be available in the suggester in the same way as now, so should reraise it as ObservationConfigError higher up (or another type that brings it into the suggestor)
There was a problem hiding this comment.
The message is good, I was thinking about the fact that _observations.py knows there exists and cli tool, while logically that information should exist higher in the stack _observations.py only knows that HISTORY_OBSERVATION is deprecated, and we can add more context higher in the stack.
There was a problem hiding this comment.
Oh, I see. I think this could be a residual followup issue, likely requires some design decisions (first idea that comes to my mind is having an own observation error type, indicating that it needs migration, and taking it from there) and moving of some things.
There was a problem hiding this comment.
This will also allow us to inject the path to the ert-file, so that the feedback message actually contains a valid command.
E.g.
ert convert_observations test-data/ert/poly_example/poly.ert instead of that placeholder.
38f4b1f to
58d1299
Compare
58d1299 to
31049ed
Compare
31049ed to
cb2eb11
Compare
cb2eb11 to
09b1e47
Compare
Avoid error from overlapping obs types
Avoid error from overlapping obs names across different response types
History observations and general observations already allow for keys being duplicate downstream, in the observation dataset. Thus, it does not make sense to allow a history obs to create 200 duplicate summary obs keys, but for the equivalent to be disallowed if explicitly done in the ert obs config.
09b1e47 to
029718f
Compare




(Goes with equinor/semeio#862)
Overview of changes
REFCASE,TIME_MAP, andHISTORY_OBSERVATIONfrom ERTREFCASEandTIME_MAPis embedded into the obs configGENERAL_OBSERVATIONcan now only be used withRESTART, usage ofDATE,HOURS,DAYScan be migrated if there is aREFCASEorTIME_MAPin the ERT config.SUMMARY_OBSERVATIONcan now only be used withDATE, usage ofRESTART,HOURS,DAYScan be migrated if there is aREFCASEorTIME_MAPin the ERT config.Nice to know:
Issue
Resolves #12619
Residual (IDEALLY NON BLOCKING) issues/TODOs
Recommend resolving before next release:
_GeneralObservation.restartnon optional Make_GeneralObservation.restartnon optional #12682Should def be fixed:
_create_observation_dataframes.pyto_observations.py. Will likely fix this in the following PR where*Observations are converted toBaseModelobjs, opening up for moving validations there (instead of doing the thedataclasses.dataclassway now) Move validations/errors wrt observations parsing #12680Not urgent:
_observations.py, ref Migrate timemap/refcase dependent observations #12629 (comment) . Handle observation errors and migrations higher in the stack #12685run_convert_observationswith just migrating the observations and taking it from there (more instances of this can be found) Replace test usages ofrun_convert_observations#12686