Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 12 additions & 0 deletions src/ert/config/observation_config_migrations.py
Original file line number Diff line number Diff line change
Expand Up @@ -684,6 +684,18 @@ def _get_restart(
has_refcase: bool,
) -> int:
if date_dict.restart is not None:
if not time_map:
raise ObservationConfigError.with_context(
f"Missing REFCASE or TIME_MAP for observations: {obs_name}",
obs_name,
)
if date_dict.restart >= len(time_map):
raise ObservationConfigError.with_context(
f"RESTART {date_dict.restart} for observation {obs_name} is "
f"out of range: the REFCASE/TIME_MAP only has "
f"{len(time_map)} report step(s).",
obs_name,
)
return date_dict.restart
if not time_map:
raise ObservationConfigError.with_context(
Expand Down
140 changes: 140 additions & 0 deletions tests/ert/unit_tests/config/test_observation_config_migrations.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
from ert.config.observation_config_migrations import (
remove_refcase_and_time_map_dependence_from_obs_config,
)
from ert.config.parsing.observations_parser import ObservationConfigError
from ert.observation_converters.history_to_summary import convert_history_to_summary


Expand Down Expand Up @@ -552,3 +553,142 @@ def test_that_history_summary_and_general_obs_are_all_migrated_together(tmp_path
};
"""
)


@pytest.mark.usefixtures("use_tmpdir")
def test_that_restart_without_refcase_or_time_map_gives_clear_error():

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We expect all errors to be clear, so it's enough to rename to e.g.:
test_that_restart_without_refcase_or_time_map_raises_obs_config_error
This will allow you to delete the redundant docstring, as it contains the same information as the title.

"""
Test that using RESTART without REFCASE or TIME_MAP raises a clear
ObservationConfigError instead of crashing with an IndexError.
"""
obs_config_path = Path("observations.txt")
obs_config_path.write_text(
dedent(
"""\
SUMMARY_OBSERVATION WOPR_OP1_9 {
VALUE = 0.1;
ERROR = 0.05;
RESTART = 9;
KEY = WOPR:OP1;
};
"""
),
encoding="utf-8",
)

config_path = Path("config.ert")
config_path.write_text(
dedent(
"""\
NUM_REALIZATIONS 1
ECLBASE ECLIPSE_CASE
OBS_CONFIG observations.txt
"""
),
encoding="utf-8",
)


@pytest.mark.usefixtures("use_tmpdir")
def test_that_restart_out_of_range_of_time_map_gives_clear_error():

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

"""
Test that a RESTART index beyond the length of TIME_MAP raises a clear
ObservationConfigError instead of crashing with an IndexError.
"""
Path("time_map.txt").write_text(
dedent(
"""\
2020-01-01
2020-01-11
2020-01-21
"""
),
encoding="utf-8",
)

obs_config_path = Path("observations.txt")
obs_config_path.write_text(
dedent(
"""\
SUMMARY_OBSERVATION WOPR_OP1_9 {
VALUE = 0.1;
ERROR = 0.05;
RESTART = 9;
KEY = WOPR:OP1;
};
"""
),
encoding="utf-8",
)

config_path = Path("config.ert")
config_path.write_text(
dedent(
"""\
NUM_REALIZATIONS 1
ECLBASE ECLIPSE_CASE
TIME_MAP time_map.txt
OBS_CONFIG observations.txt
"""
),
encoding="utf-8",
)

with pytest.raises(ObservationConfigError):
convert_history_to_summary(str(config_path))


@pytest.mark.usefixtures("use_tmpdir")
def test_that_restart_with_refcase_present_converts_successfully():

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. We try to avoid wording like "successfully", as that is too vague to understand what is being asserted.

  2. I want you to make sure this is not a redundant test. What is being tested here which is not already tested in the suite?

"""
Regression test: RESTART with a valid REFCASE (restart index within
range) should convert cleanly, not raise.
"""
smspec, unsmry = create_summary_smspec_unsmry(
summary_vectors={"FOPR": [100, 110, 120]},
start_date=datetime(2020, 1, 1), # ruff: ignore[call-datetime-without-tzinfo]
)
smspec.to_file(Path("REFCASE.SMSPEC"))
unsmry.to_file(Path("REFCASE.UNSMRY"))

obs_config_path = Path("observations.txt")
obs_config_path.write_text(
dedent(
"""\
SUMMARY_OBSERVATION FOPR_OBS {
VALUE = 110.0;
ERROR = 5.0;
RESTART = 1;
KEY = FOPR;
};
"""
),
encoding="utf-8",
)

config_path = Path("config.ert")
config_path.write_text(
dedent(
"""\
NUM_REALIZATIONS 1
ECLBASE ECLIPSE_CASE
REFCASE REFCASE
OBS_CONFIG observations.txt
"""
),
encoding="utf-8",
)

# Should not raise
convert_history_to_summary(str(config_path))

assert (
Path("observations.txt").read_text(encoding="utf-8")
== """SUMMARY_OBSERVATION FOPR_OBS {
VALUE = 110.0;
ERROR = 5.0;
DATE = 2020-01-01;
KEY = FOPR;
};
"""
)