Allow use of wildcards in definition of seismic observations/responses - #14118
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for glob-style wildcards when defining seismic observations and seismic response input files, reducing configuration duplication for families of sim2seis-generated CSVs. The change centralizes shared seismic utilities (tolerance checks, response/observation alignment, and pattern-to-filepath resolution) and updates config + GUI/storage call sites accordingly.
Changes:
- Introduce
SeismicDatautilities, including filename-only wildcard expansion and shared tolerance/location alignment helpers. - Update seismic response/observation loading paths to support wildcard patterns and reuse shared utilities.
- Extend/adjust unit tests to cover wildcard expansion and multi-realization visualization behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ert/unit_tests/gui/ertwidgets/test_ensemble_widget.py | Refactors response-plot assertions and updates test to cover multiple seismic responses. |
| tests/ert/unit_tests/config/test_seismic_config.py | Updates tests to use real temp files and adds coverage for wildcard patterns in SEISMIC response inputs. |
| tests/ert/unit_tests/config/test_observation_declaration.py | Adds coverage for wildcard patterns (and literal metacharacters) in seismic observation CSV paths. |
| src/ert/storage/local_ensemble.py | Switches seismic response/observation alignment helper to SeismicData. |
| src/ert/gui/tools/manage_experiments/ensemble_widget.py | Switches seismic response/observation alignment helper to SeismicData. |
| src/ert/config/seismic_config.py | Uses wildcard expansion for seismic response file discovery and shared tolerance logic. |
| src/ert/config/_reservoir_data_utils.py | Adds SeismicData utilities (pattern resolution, KDTree proximity checks, and alignment helper). |
| src/ert/config/_observations.py | Enables wildcard expansion for seismic observation CSV loading via SeismicData.resolve_pattern_filepaths. |
Suppressed comments (1)
src/ert/config/_reservoir_data_utils.py:128
resolve_pattern_filepaths()returns matches in filesystem iteration order (Path.iterdir()), which is not guaranteed to be deterministic. Sorting the matched paths (e.g., by filename) would make response/observation key resolution stable across platforms and runs.
compiled_pattern = re.compile(fnmatch.translate(filename_pattern))
matching_paths = [
path
for path in search_dir.iterdir()
if path.is_file() and compiled_pattern.fullmatch(path.name)
]
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #14118 +/- ##
==========================================
- Coverage 91.93% 91.92% -0.01%
==========================================
Files 482 483 +1
Lines 33516 33556 +40
==========================================
+ Hits 30813 30848 +35
- Misses 2703 2708 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
980f466 to
7bbe069
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/ert/config/_reservoir_data_utils.py:122
resolve_pattern_filepaths()only checkssearch_dir.exists(). Ifbasedir/pattern_path.parentexists but is a file (not a directory), the lateriterdir()call will raiseNotADirectoryErrorinstead of the intendedon_error(...). Similarly, the non-glob branch should ensure the resolved path is a file (not justexists()) so directory paths don’t get treated as response/observation files.
search_dir = Path(basedir) / pattern_path.parent
if not search_dir.exists():
raise on_error(
f"Directory '{search_dir.absolute()}' does not exist "
"or is not accessible."
)
src/ert/config/_reservoir_data_utils.py:129
- The wildcard branch returns
matching_pathsin the filesystem’siterdir()order, which is not guaranteed to be stable across platforms/filesystems. Sorting the matches (e.g., by filename) makes response/observation ordering deterministic and avoids potential order-dependent behavior/flaky tests.
matching_paths = [
path
for path in search_dir.iterdir()
if path.is_file() and compiled_pattern.fullmatch(path.name)
]
ajaust
left a comment
There was a problem hiding this comment.
The changes look good. 🙂
I only have some minor comments and questions.
|
Should we also add the wildcard support somewhere to the documentation? |
7bbe069 to
e957431
Compare
achaikou
left a comment
There was a problem hiding this comment.
Thanks for the review!
Unfortunately, due to severe merge conflict I had to rebase first and do significant changes in the last 2 commits.
Mostly changes affected test_seismic_config.
The docs I plan to add/go through before the feature is officially released, which apparently should be this month now 😄.
ajaust
left a comment
There was a problem hiding this comment.
Looks good overall. I looked especially into the two last commits. I have only a few questions/remarks. 🙂
As it has been pointed out, we start having too much duplication between observations and config, yet we do not have a good place to put it into. So creating a new file to keep common logic. ReservoirData was chosen as a general term to describe RFT/Seismic/Summary response or observation data.
As there was no constrain on realizations, all realizations were joined with all responses, resulting in too many dots on the plot.
Changes to other tests are mostly caused by additional check for directory existence that doesn't play nicely with mocked_files.
061b699 to
84fdd0d
Compare
Issue
Resolves #13913
Approach
It has been confirmed that we want wildcards, not regex.
git rebase -i main --exec 'just rapid-tests')When applicable
merge screenshot-PR in ert-testdata before merging this PR.