-
Notifications
You must be signed in to change notification settings - Fork 10
Feature: validator checks and updates parameters according to nlayer #731
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature: validator checks and updates parameters according to nlayer #731
Conversation
This script generates SUEWS sample configuration files for different nlayer values (1-7) by: - Reading a base sample config file - Updating the nlayer value - Adjusting vertical layer arrays to match the new nlayer - Updating roofs/walls arrays in properties and initial_states The script will be used to maintain consistency across sample configs and enable automated validation of nlayer-specific structures. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Co-authored-by: dayantur <[email protected]>
🤖 I've automatically formatted the code in this PR using:
Please pull the latest changes before making further edits. |
…er required dimension
🔍 Schema Preview DeployedPreview URLs:
Production URLs (unchanged):
|
Co-authored-by: dayantur <[email protected]>
🤖 I've automatically formatted the code in this PR using:
Please pull the latest changes before making further edits. |
…mple-config' of https://github.com/UMEP-dev/SUEWS into dayantur/feature/validator/automatic-nlayer-standard-sample-config
Co-authored-by: dayantur <[email protected]>
🤖 I've automatically formatted the code in this PR using:
Please pull the latest changes before making further edits. |
…mple-config' of https://github.com/UMEP-dev/SUEWS into dayantur/feature/validator/automatic-nlayer-standard-sample-config
Co-authored-by: dayantur <[email protected]>
🤖 I've automatically formatted the code in this PR using:
Please pull the latest changes before making further edits. |
…mple-config' of https://github.com/UMEP-dev/SUEWS into dayantur/feature/validator/automatic-nlayer-standard-sample-config
Co-authored-by: dayantur <[email protected]>
🤖 I've automatically formatted the code in this PR using:
Please pull the latest changes before making further edits. |
…mple-config' of https://github.com/UMEP-dev/SUEWS into dayantur/feature/validator/automatic-nlayer-standard-sample-config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question about :
This script appears to be unused in the codebase:
- Not imported or referenced anywhere in the code
- The generated files ( - ) don't exist in the repo
- Only exists in
Was this a development/testing utility that was accidentally committed? If so, I'd suggest removing it from the PR to keep the repository clean.
If the script is intended for future use or documentation purposes, consider:
- Moving it to or directory
- Adding a README explaining its purpose
- Or converting it to a pytest fixture in
What was the intended use case for this script?
Sorry @sunt05 - that should be removed! |
Thanks @dayantur ! Please see my comment above about the script at the root level. |
Resolved CHANGELOG.md conflict by merging both: - PR731's nlayer validation feature - Master's recent UMEP/QGIS build improvements and other changes Updated annual statistics: 36 features, 119 total entries for 2025 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Co-authored-by: dayantur <[email protected]>
🤖 I've automatically formatted the code in this PR using:
Please pull the latest changes before making further edits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Summary
Excellent work on implementing automatic nlayer dimension validation! This PR addresses issue #712 comprehensively with a well-designed solution. All 5 new tests pass successfully.
Strengths
- Comprehensive validation logic: Correctly handles all array types (simple arrays, nlayer+1 arrays, and complex nested structures)
- Smart template generation: The
create_null_roof_wall_template()
function intelligently replicates complex nested structures for roofs/walls - Clear error reporting: The report format clearly distinguishes between array types and provides actionable fix suggestions
- Excellent test coverage: 5 comprehensive tests covering all scenarios (simple arrays, multiple errors, complex structures, and no-error cases)
- Safe defaults:
detect_nlayer_from_user_yaml()
gracefully handles errors and defaults to 3
Minor Observations
- Code formatting change:
get_ver_git.py
has a formatting change (black/ruff auto-format) that's unrelated to the feature - this is fine but worth noting - Silent error handling:
detect_nlayer_from_user_yaml()
silently defaults to 3 on all errors. This is probably fine, but consider if logging might help debugging in some cases - RefValue handling: The code correctly handles both RefValue format (
{"value": ...}
) and direct values - well done on this complexity
Suggestions for Future Enhancement (not blocking)
- Consider adding a test that validates the actual error report format/content (checking the report string generation)
- The null template creation for roofs/walls is impressive - might be worth documenting this approach in code comments for future maintainers
Verdict
✅ Approve - This PR is ready to merge. The implementation is solid, well-tested, and addresses the problem described in #712 effectively.
…r-standard-sample-config
🤖 I've automatically formatted the code in this PR using:
Please pull the latest changes before making further edits. |
Co-authored-by: dayantur <[email protected]>
hi @sunt05 :) This is just to let you know that after your (positive!) review, I found a tiny bug this morning - I already fixed that and now everything seems to work correctly. |
Thanks @dayantur - looks there is some conflict with master please fix then we can merge! Good job! |
Problem
This PR proposes a solution for the problem discussed in #712 . In fact, different parameters arrays have different dimension requirements:
height
: nlayer + 1 elementsveg_frac
,veg_scale
,building_frac
,building_scale
: nlayer elementsroofs
,walls
(in bothvertical_layers
andinitial_states
): nlayer elements with complex nested structuresNo validation existed to detect these mismatches at configuration time, and currently (if nlayer user < nlayer standard config), the validator "fills up" the remaining dimensions with dummy values.
Main Changes
orchestrator.py
withdetect_nlayer_from_user_yaml()
to extract nlayer value from user configurationphase_a.py
withvalidate_nlayer_dimensions()
(core validation logic for all array types),create_null_roof_wall_template()
(replicates complex nested roof/wall structures with null values),annotate_missing_parameters()
andcreate_analysis_report()
.Testing
Added comprehensive tests in
test/data_model/test_validation.py
.Solution
Implemented Phase A validation that:
If nlayer user < nlayer sample config:
nlayer
from user configuration automatically- Simple arrays: pads with None values
- Complex structures (roofs/walls): creates complete null template structures matching reference element
If nlayer user > nlayer sample config:
nlayer
from user configuration automaticallyReport Format
Clear messages distinguish between array types and levels: