Feat: check SUEWS dectreeh/evetreeh consistency with SPARTACUS veg_scale/veg_frac#1222
Feat: check SUEWS dectreeh/evetreeh consistency with SPARTACUS veg_scale/veg_frac#1222
Conversation
CI Build PlanChanged FilesPython source (2 files)
Tests (4 files)
Documentation (3 files)
Build Configuration
Rationale
Updated by CI on each push. See path-filters.yml for category definitions. |
Preview Deployed
Note This preview is ephemeral. It will be lost when:
To restore, push any commit to this PR. |
dayantur
changed the title
dayantur
changed the title
|
hi @yiqing1021 :) the only test failing is the stebbs one (test_stebbs_building_energy_outputs). dectreeh and evetreeh cannot be taller (13.1 m at the moment) than SPARTACUS top layer (11 m at the moment). Can you please have a look at you steebs sample config, change the parameters and try to make this test pass? Here to help with anything if needed :) many thanks! |
|
hi @sunt05 - this PR is ready for review :) please have a look once you have time - especially the tests, since I had to change one of the .nml to make them green |
sunt05
left a comment
sunt05
left a comment
There was a problem hiding this comment.
Thanks for adding this validation, @dayantur — ensuring vegetation arrays are physically consistent with tree heights is a valuable check for SPARTACUS users. The orchestration wiring and guard clauses follow the established patterns well, and the fixture updates (GridLayouttest.nml, stebbs_test) are sensible corrections.
Two things need addressing before merge:
The boundary comparison on line 1720 uses strict <, which means that when a tree height exactly equals a layer boundary (e.g. max_tree=15, height=[0, 5, 10, 15, 20]), the layers above the tree go unchecked. Changing to <= fixes this.
The five new tests all pass by hitting the early guard return (no land_cover with tree heights in the stubs), so the core layer-checking loop is never exercised. We need at least one test where trees are present and vegetation is correctly zero above the tree layer, and one where it isn't.
Two minor points are noted in the inline comments below.
| # Find the last layer where max_tree < height[layer] (layer index 1..nlayer) | ||
| layer_index = None | ||
| for i in range(1, len(height_arr)): | ||
| if max_tree < height_arr[i]: |
There was a problem hiding this comment.
This should be <= rather than <. With strict <, when max_tree exactly equals a layer boundary height, the code picks the wrong layer_index and skips checking layers above the tree.
Example: max_tree=15, height=[0, 5, 10, 15, 20]
- With
<: findslayer_index=4(atheight[4]=20),range(4, 4)=[]— layer 4 (15–20 m) goes unchecked - With
<=: findslayer_index=3(atheight[3]=15),range(3, 4)=[3]— correctly checks layer 4
| if max_tree < height_arr[i]: | |
| if max_tree <= height_arr[i]: |
| if isinstance(arr, (list, tuple)): | ||
| for i in range(layer_index, nlayer): | ||
| val = arr[i] | ||
| if val != 0: |
There was a problem hiding this comment.
Minor: val != 0 uses exact float equality. The adjacent _validate_spartacus_sfr uses np.isclose(..., atol=tol) for the same kind of check (lines 1671, 1680). Worth aligning for floating-point robustness:
| if val != 0: | |
| if not np.isclose(val, 0, atol=1e-6): |
| """ | ||
| Check that veg_scale and veg_frac are zero above the layer where max_tree falls. | ||
| max_tree = max(dectreeh, evetreeh) | ||
| The last layer where max_tree < height[layer] (layer index 1..nlayer) is the tree layer. |
There was a problem hiding this comment.
Nit: the docstring says "The last layer where max_tree < height[layer]" but the loop finds the first such layer (it uses break). Suggest changing to "The first layer where…".
| def test_validate_spartacus_veg_dimensions_valid(): | ||
| """Test validate_spartacus_veg_dimensions passes with matching veg_frac and nlayer.""" | ||
| cfg = SUEWSConfig.model_construct() | ||
| vertical_layers = SimpleNamespace( | ||
| nlayer=2, | ||
| veg_frac=[0.3, 0.7], | ||
| ) | ||
| props = SimpleNamespace(vertical_layers=vertical_layers) | ||
| site = SimpleNamespace(properties=props, name="TestSite") | ||
| msgs = cfg._validate_spartacus_veg_dimensions(site, 0) | ||
| assert msgs == [] | ||
|
|
||
| def test_validate_spartacus_veg_dimensions_too_few_elements(): | ||
| """Test validate_spartacus_veg_dimensions detects too few veg_frac elements.""" | ||
| cfg = SUEWSConfig.model_construct() | ||
| vertical_layers = SimpleNamespace( | ||
| nlayer=3, | ||
| veg_frac=[0.2, 0.8], | ||
| ) | ||
| props = SimpleNamespace(vertical_layers=vertical_layers) | ||
| site = SimpleNamespace(properties=props, name="TestSite") | ||
| msgs = cfg._validate_spartacus_veg_dimensions(site, 0) | ||
| assert msgs == [] | ||
|
|
||
| def test_validate_spartacus_veg_dimensions_too_many_elements(): | ||
| """Test validate_spartacus_veg_dimensions detects too many veg_frac elements.""" | ||
| cfg = SUEWSConfig.model_construct() | ||
| vertical_layers = SimpleNamespace( | ||
| nlayer=2, | ||
| veg_frac=[0.1, 0.2, 0.7], | ||
| ) | ||
| props = SimpleNamespace(vertical_layers=vertical_layers) | ||
| site = SimpleNamespace(properties=props, name="TestSite") | ||
| msgs = cfg._validate_spartacus_veg_dimensions(site, 0) | ||
| assert msgs == [] | ||
|
|
||
| def test_validate_spartacus_veg_dimensions_missing_veg_frac(): | ||
| """Test validate_spartacus_veg_dimensions handles missing veg_frac gracefully.""" | ||
| cfg = SUEWSConfig.model_construct() | ||
| vertical_layers = SimpleNamespace( | ||
| nlayer=2, | ||
| # veg_frac missing | ||
| ) | ||
| props = SimpleNamespace(vertical_layers=vertical_layers) | ||
| site = SimpleNamespace(properties=props, name="TestSite") | ||
| msgs = cfg._validate_spartacus_veg_dimensions(site, 0) | ||
| assert msgs == [] | ||
|
|
||
| def test_validate_spartacus_veg_dimensions_missing_nlayer(): | ||
| """Test validate_spartacus_veg_dimensions handles missing nlayer gracefully.""" | ||
| cfg = SUEWSConfig.model_construct() | ||
| vertical_layers = SimpleNamespace( | ||
| veg_frac=[0.5, 0.5], | ||
| # nlayer missing | ||
| ) | ||
| props = SimpleNamespace(vertical_layers=vertical_layers) | ||
| site = SimpleNamespace(properties=props, name="TestSite") | ||
| msgs = cfg._validate_spartacus_veg_dimensions(site, 0) | ||
| assert msgs == [] |
There was a problem hiding this comment.
All five tests pass via the early if not tree_heights: return guard because none of the stubs include land_cover with dectreeh/evetreeh. The core loop (config.py L1718–1743) is never reached.
Suggested additions:
- Passing case: stub with
dectreeh=12,height=[0, 5, 10, 15, 20],veg_frac=[0.3, 0.3, 0.2, 0, 0]— should return[] - Failing case: same but
veg_frac=[0.3, 0.3, 0.2, 0.1, 0]— should flagveg_frac[3] - Boundary case:
max_treeexactly on a layer boundary - Exceeds-all case:
max_tree=100withheight=[0, 5, 10]— should produce the "exceeds" message
3 participants
This PR addresses #1204 and introduces a new conditional validation rule to ensure vertical consistency between urban vegetation layers and tree heights in SUEWS when SPARTACUS is enabled.
It adds logic to check that
veg_fracandveg_scalearrays are zero or null above the layer where trees end.Main Changes