GEOPY-2604: add validation to exclude plate-simulation and depth of investigation…#395
GEOPY-2604: add validation to exclude plate-simulation and depth of investigation…#395benk-mira wants to merge 1 commit intorelease/GA_4.8from
Conversation
There was a problem hiding this comment.
Pull request overview
Adds input validation to the tile estimation workflow to prevent unsupported SimPEGGroup targets (plate simulation and depth of investigation) from being used with the tile estimator.
Changes:
- Added a Pydantic field validator on
TileParameters.simulationto rejectplate_simulationanddepth_of_investigationrun commands. - Added a new test asserting that plate simulation groups are rejected during
TileParametersvalidation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
simpeg_drivers/utils/tile_estimate.py |
Introduces validation logic to exclude unsupported simulation targets for tile estimation. |
tests/utils_tile_estimate_test.py |
Adds a regression test for the new simulation-target validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_simulation_validation_rejects_plate_simulation(tmp_path): | ||
| simulation = MagicMock(spec=SimPEGGroup) | ||
| simulation.options = { | ||
| "run_command": "simpeg_drivers.plate_simulation.driver", | ||
| "title": "Plate Simulation", | ||
| } | ||
|
|
||
| with Workspace.create(tmp_path / "test.geoh5") as geoh5: | ||
| with pytest.raises(ValidationError, match="not a valid target"): | ||
| TileParameters(geoh5=geoh5, simulation=simulation) |
There was a problem hiding this comment.
This only tests rejection of plate_simulation. Since the new validator also rejects depth_of_investigation (and should still accept valid forward/inverse drivers), add tests covering at least one depth_of_investigation.* run_command and one allowed run_command to avoid regressions.
| def test_simulation_validation_rejects_plate_simulation(tmp_path): | |
| simulation = MagicMock(spec=SimPEGGroup) | |
| simulation.options = { | |
| "run_command": "simpeg_drivers.plate_simulation.driver", | |
| "title": "Plate Simulation", | |
| } | |
| with Workspace.create(tmp_path / "test.geoh5") as geoh5: | |
| with pytest.raises(ValidationError, match="not a valid target"): | |
| TileParameters(geoh5=geoh5, simulation=simulation) | |
| @pytest.mark.parametrize( | |
| ("run_command", "title"), | |
| [ | |
| ("simpeg_drivers.plate_simulation.driver", "Plate Simulation"), | |
| ( | |
| "simpeg_drivers.depth_of_investigation.driver", | |
| "Depth of Investigation", | |
| ), | |
| ], | |
| ) | |
| def test_simulation_validation_rejects_invalid_targets(tmp_path, run_command, title): | |
| simulation = MagicMock(spec=SimPEGGroup) | |
| simulation.options = { | |
| "run_command": run_command, | |
| "title": title, | |
| } | |
| with Workspace.create(tmp_path / "test.geoh5") as geoh5: | |
| with pytest.raises(ValidationError, match="not a valid target"): | |
| TileParameters(geoh5=geoh5, simulation=simulation) | |
| def test_simulation_validation_accepts_valid_target(tmp_path): | |
| simulation = MagicMock(spec=SimPEGGroup) | |
| simulation.options = { | |
| "run_command": "simpeg_drivers.electricals.direct_current.three_dimensions.driver", | |
| "title": "Direct Current 3D", | |
| } | |
| with Workspace.create(tmp_path / "test.geoh5") as geoh5: | |
| params = TileParameters(geoh5=geoh5, simulation=simulation) | |
| assert params.simulation is simulation |
| @field_validator("simulation", mode="before") | ||
| @classmethod | ||
| def forward_and_inverse_drivers_only(cls, value): | ||
| run_command = value.options["run_command"] | ||
| invalid = ["plate_simulation", "depth_of_investigation"] | ||
| if any(k in run_command for k in invalid): | ||
| title = value.options["title"] |
There was a problem hiding this comment.
The validator assumes simulation.options always contains run_command and title and indexes them directly. If a SimPEGGroup is missing either key (or options is None/not a dict), this will raise a KeyError/AttributeError and produce an unhelpful ValidationError instead of the intended message. Consider using defensive access (e.g., .get) and raising a clear ValueError when required option keys are absent; also consider running this check after parsing so simulation has been resolved to a SimPEGGroup instance.
| @field_validator("simulation", mode="before") | |
| @classmethod | |
| def forward_and_inverse_drivers_only(cls, value): | |
| run_command = value.options["run_command"] | |
| invalid = ["plate_simulation", "depth_of_investigation"] | |
| if any(k in run_command for k in invalid): | |
| title = value.options["title"] | |
| @field_validator("simulation", mode="after") | |
| @classmethod | |
| def forward_and_inverse_drivers_only(cls, value): | |
| options = getattr(value, "options", None) | |
| if not isinstance(options, dict): | |
| raise ValueError( | |
| "Simulation options must be provided as a dictionary containing " | |
| "'run_command' and 'title'." | |
| ) | |
| run_command = options.get("run_command") | |
| if not isinstance(run_command, str) or not run_command: | |
| raise ValueError( | |
| "Simulation options must include a non-empty 'run_command'." | |
| ) | |
| title = options.get("title") | |
| if not isinstance(title, str) or not title: | |
| raise ValueError("Simulation options must include a non-empty 'title'.") | |
| invalid = ["plate_simulation", "depth_of_investigation"] | |
| if any(k in run_command for k in invalid): |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/GA_4.8 #395 +/- ##
==================================================
+ Coverage 90.17% 90.20% +0.03%
==================================================
Files 129 129
Lines 6474 6484 +10
Branches 817 818 +1
==================================================
+ Hits 5838 5849 +11
Misses 422 422
+ Partials 214 213 -1
🚀 New features to boost your workflow:
|
GEOPY-2604 - Validate incorrect SimPEGGroups in tile estimator and fail gracefully
… SimPEGGroups