Add validation of parsed general input - #950
Conversation
c69b9cc to
44a9616
Compare
|
I'm thinking the next step would be to create a class for GeneralInput to ease the validation and reduce the amount of dictionaries floating around. |
There was a problem hiding this comment.
Pull request overview
Adds an early validation step for the “general input” sheet values parsed from Excel, aiming to produce more relevant validation errors before the config dict is further processed.
Changes:
- Introduces
validate_general_inputand supporting validation/error types for raw general input. - Moves
resolve_pathintofmudesign.utilsand updates Excel parsing to use it. - Updates tests: adds a dedicated general-input validation test suite and removes some seed-strategy tests from config validation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/fmudesign/test_general_input_validation.py | New test coverage for early/raw general input validation behavior. |
| tests/fmudesign/test_config_validation.py | Removes seed-strategy validation tests that are now expected to be covered earlier. |
| src/semeio/fmudesign/utils.py | Adds shared resolve_path helper (moved from _excel_to_dict). |
| src/semeio/fmudesign/general_input_validation.py | New raw general input validator and related error types. |
| src/semeio/fmudesign/config_validation.py | Simplifies validation/normalization logic (now relies more on earlier validation). |
| src/semeio/fmudesign/_excel_to_dict.py | Invokes new validate_general_input after parsing the general input sheet. |
Suppressed comments (2)
src/semeio/fmudesign/config_validation.py:43
validate_configurationno longer coerces/validatescorrelation_iterations(anddistribution_seed) when provided. Since downstream code comparescorrelation_iterations > 0and seeds numpy RNGs, leaving these as strings/floats can cause runtimeTypeErroreven if earlier validation succeeded.
key = "correlation_iterations"
if key not in config:
if verbosity > 0:
print(f"{key!r} not set in general input sheet. Setting to default 0.")
print(" - When set to 0, Iman Conover is used to induce correlations.")
tests/fmudesign/test_general_input_validation.py:299
- Typo in test name: "sesntivity" -> "sensitivity".
def test_seed_strategy_does_not_raise_given_case_sesntivity(seed_strategy):
_setup_and_validate_config({"seed_strategy": seed_strategy})
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
70460d0 to
20b8880
Compare
a8b69d8 to
eb3b4f8
Compare
192b260 to
7129472
Compare
|
I've reworked the approach quite a bit from previous feedback, ended up squashing the fixups as I didn't bother constantly rebasing 12 fixup commits 😅 |
c37da17 to
0824e23
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
src/semeio/fmudesign/utils.py:268
- The documented fallback is not implemented: a missing reference ending in
xlsxorcsvreaches line 290 and raisesValueErrorinstead of returningreference. Update the contract so callers know that unresolved file-like references raise.
def resolve_path(input_filename: str, reference: str | None) -> str | None:
"""The path `input_filename` is an Excel sheet, and `reference` is a cell
value that *might* be a reference to another file. Resolve the path to
`reference` and return. If no such file exists, return `reference`.
"""
Creating new files quickly create circular dependencies with _excel_to_dict.py. Spreading these functions accross multiple files eases the dependency problems.
|
The only issue I'm aware of now is the validation message of rms_seeds. The message is fine, but the error type is weird: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
Suppressed comments (1)
src/semeio/fmudesign/utils.py:268
- The docstring still says an unresolved reference is returned, but a missing
.csv/.xlsxreference reaches line 290 and raisesValueError. Document the extension-dependent behavior and exception so callers can rely on the actual contract.
def resolve_path(input_filename: str, reference: str | None) -> str | None:
"""The path `input_filename` is an Excel sheet, and `reference` is a cell
value that *might* be a reference to another file. Resolve the path to
`reference` and return. If no such file exists, return `reference`.
"""
|
|
||
| @field_serializer("rms_seeds", "background") | ||
| def serialize_paths(self, field: Path) -> str: # ruff: ignore[no-self-use] | ||
| return str(field) |
There was a problem hiding this comment.
probably need an if check to only convert if is not None
There was a problem hiding this comment.
Good call, also wrote test for this now.
| distribution_seed: NonNegativeInt | None | ||
| rms_seeds: FilePath | Literal["default"] | None | ||
| correlation_iterations: NonNegativeInt = 0 | ||
| seed_strategy: SeedStrategy = SeedStrategy.JOINT |
There was a problem hiding this comment.
Due to pydantics use_enum_values=True i think the default will differ from inputs, can set Field( default=SeedStrategy.JOINT, validate_default=True, )
to get the same behaviour in both cases.
There was a problem hiding this comment.
Also good call, wrote test for this as well.
Should be fine for now. |
The existing validation is validating the config dict generated by various steps when parsing the excel file to a config dict.
It makes sense to have a validation step earlier in the loop which validates the raw input from the excel file to give a more relevant exception feedback to unchanged values.
This validation should not edit any values, only validate the input - except for validation of existing files relative to the excel
input_filename