Restore observed soil moisture forcing (GH-3)#859
Merged
Conversation
sunt05
temporarily deployed
to
github-pages-preview
GitHub Actions
Inactive
|
🤖 I've automatically formatted the code in this PR using:
Please pull the latest changes before making further edits. |
Preview Deployed
Note This preview is ephemeral. It will be lost when:
To restore, push any commit to this PR. |
sunt05
temporarily deployed
to
github-pages-preview
GitHub Actions
Inactive
sunt05
temporarily deployed
to
github-pages-preview
GitHub Actions
Inactive
|
🤖 I've automatically formatted the code in this PR using:
Please pull the latest changes before making further edits. |
sunt05
temporarily deployed
to
github-pages-preview
GitHub Actions
Inactive
|
🤖 I've automatically formatted the code in this PR using:
Please pull the latest changes before making further edits. |
sunt05
force-pushed
the
sunt05/gh3-xsmd-input
branch
from
26d43ac to
81626bf
Compare
sunt05
temporarily deployed
to
github-pages-preview
GitHub Actions
Inactive
sunt05
temporarily deployed
to
github-pages-preview
GitHub Actions
Inactive
sunt05
temporarily deployed
to
github-pages-preview
GitHub Actions
Inactive
sunt05
force-pushed
the
sunt05/gh3-xsmd-input
branch
from
cce8efe to
6b605ca
Compare
sunt05
temporarily deployed
to
github-pages-preview
GitHub Actions
Inactive
sunt05
force-pushed
the
sunt05/gh3-xsmd-input
branch
from
6b605ca to
95ea7ca
Compare
sunt05
temporarily deployed
to
github-pages-preview
GitHub Actions
Inactive
sunt05
force-pushed
the
sunt05/gh3-xsmd-input
branch
from
95ea7ca to
0ede9a4
Compare
sunt05
temporarily deployed
to
github-pages-preview
GitHub Actions
Inactive
sunt05
temporarily deployed
to
github-pages-preview
GitHub Actions
Inactive
sunt05
force-pushed
the
sunt05/gh3-xsmd-input
branch
from
fbba5b4 to
34a4d53
Compare
sunt05
temporarily deployed
to
github-pages-preview
GitHub Actions
Inactive
sunt05
force-pushed
the
sunt05/gh3-xsmd-input
branch
from
34a4d53 to
babe08e
Compare
sunt05
temporarily deployed
to
github-pages-preview
GitHub Actions
Inactive
sunt05
force-pushed
the
sunt05/gh3-xsmd-input
branch
from
babe08e to
82129c1
Compare
sunt05
temporarily deployed
to
github-pages-preview
GitHub Actions
Inactive
sunt05
temporarily deployed
to
github-pages-preview
GitHub Actions
Inactive
sunt05
force-pushed
the
sunt05/gh3-xsmd-input
branch
from
c88cbac to
d1e0980
Compare
sunt05
temporarily deployed
to
github-pages-preview
GitHub Actions
Inactive
Co-authored-by: sunt05 <1802656+sunt05@users.noreply.github.com>
Code review fixes addressing Python compatibility, documentation clarity, and test coverage: - Fix type hints: Use Optional[float] instead of float | None for Python 3.9+ compatibility - Document surface type indices: Add comprehensive comment explaining SUEWS surface types 0-5 - Document missing value tolerance: Explain +1.0 offset for -999 missing value detection - Add dimensional analysis: Document unit conversions for volumetric and gravimetric methods - Add debug logging: Log conversion statistics (min/max/mean deficit, valid count) at DEBUG level - Improve error messages: Make metadata mismatch error more actionable with specific solutions - Add edge case tests: NaN handling, clipping validation, negative values, all-missing scenario - Fix build configuration: Add _soil_obs.py to meson.build for proper module installation All tests pass (7/7): - test_convert_observed_soil_moisture_volumetric - test_convert_observed_soil_moisture_gravimetric - test_missing_metadata_raises_error - test_xsmd_with_nan_values - test_xsmd_exceeding_smcap - test_xsmd_negative_values - test_xsmd_all_missing
Co-authored-by: sunt05 <1802656+sunt05@users.noreply.github.com>
Fixes table conversion and CLI tests by handling optional soil observation fields gracefully: **Problem**: Old SUEWS tables don't have the new soil observation metadata fields (soildensity, obs_sm_depth, obs_sm_cap, obs_soil_not_rocks), causing conversion failures. **Solution**: 1. `SurfaceProperties.from_df_state()`: Check if column exists before accessing; set to None for optional fields that are missing (backwards compatible with old tables) 2. `SurfaceProperties.to_df_state()`: Use -999 (missing value) for soil observation fields when None, instead of 0.0 which would fail validation 3. `test_df_state_conversion.py`: Update expected column count from 1423 to 1451 (28 new columns = 4 fields × 7 surface types) **Tests fixed** (8/8 passing): - test_convert_old_csv - test_single_layer_end_to_end - test_multi_layer_end_to_end - test_legacy_version_auto_detection[2020a] - test_legacy_version_auto_detection[2019a] - test_legacy_version_auto_detection[2018a] - (2 additional CLI conversion tests) **Behaviour**: New optional fields default to -999 (missing) when loading old tables, which is correct since soil observations are only needed when SMDMethod > 0.
Co-authored-by: sunt05 <1802656+sunt05@users.noreply.github.com>
Reorganises soil observation conversion logic following SuPy's established pattern where specialized utilities live in the `util/` subdirectory. **Changes**: - Moved `_soil_obs.py` → `util/_forcing.py` - Updated import in `_run.py`: `from ._soil_obs` → `from .util._forcing` - Updated `meson.build` to reflect new file location - Updated test imports to load from new location - No functional changes; purely organisational refactoring **Rationale**: - Removes module from top-level namespace - Follows established pattern (util/_atm.py, util/_ohm.py, etc.) - Improves code organisation whilst maintaining testability **Testing**: - Direct module loading confirms functionality (Python tests pass) - Note: Full test suite blocked by unrelated Fortran build issue
- Replace convoluted importlib mechanics with standard import in test_soil_obs_conversion.py (reduces 14 lines to 3) - Clarify df_state column count comment to explain the 28 soil observation fields (4 fields × 7 surfaces) - Note that Water/Bldgs surfaces don't need soil obs but get columns anyway due to flat df_state structure 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Observed soil moisture is a point measurement, so per-surface weighted averaging was over-engineered. Now reads metadata from surface 0 only. Changes: - Remove _surface_weights() and _weighted_average() functions - Add SOIL_OBS_SURFACE_INDEX constant (=0) for single-surface read - Fix NaN handling: use fillna(0) before astype(int) for smdmethod - Update tests to use single-surface helper - Update docs to specify Paved surface only - Correct CHANGELOG date (13 Nov -> 14 Nov) Code reduction: ~35 net lines removed 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The formula is numerically correct but the dimensional analysis comment was misleading. Clarified that division by ρ_water (1 g/cm³) is implicit. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Previously, metadata was hardcoded to surface 0 (Paved), which doesn't make sense if Paved has zero coverage. Now searches surfaces 0-5 and uses the first one with complete metadata, allowing users to set observation parameters on grass, bare soil, or any other surface. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Soil observation metadata is now correctly modelled as a site-level property rather than being awkwardly stored per-surface. This reflects the physical reality that observed soil moisture is a point measurement. Changes: - Add SoilObservationConfig class to site.py with depth, smcap, soil_not_rocks, and bulk_density fields - Add soil_observation field to SiteProperties - Update _forcing.py to check site-level config first, fall back to per-surface for backwards compatibility - Add 4 new tests for site-level config and precedence behaviour - Update documentation with preferred YAML approach 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Co-authored-by: sunt05 <1802656+sunt05@users.noreply.github.com>
Simplify soil observation configuration to only support the site-level YAML approach via `site.properties.soil_observation`. This removes: - Per-surface fallback in _extract_soil_obs_metadata() - Per-surface soil observation fields from SurfaceProperties - Legacy per-surface tests from test_soil_obs_conversion.py - Legacy documentation from SUEWS_Soil.rst The per-surface approach via SUEWS_Soil.txt columns is now fully deprecated in favour of the YAML-only configuration. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Co-authored-by: sunt05 <1802656+sunt05@users.noreply.github.com>
The soil observation configuration is now documented exclusively in the auto-generated YAML config reference (from SoilObservationConfig Pydantic model in site.py). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
These per-surface soil observation columns are no longer supported. Soil observation configuration is now site-level only via YAML. Removed from: - Input_Options.rst: OBS_SMCap, OBS_SMDepth, OBS_SoilNotRocks entries - csv-table/: OBS_SM*.csv files deleted - SUEWS_Soil.csv: columns 7-9 removed - sample-table/SUEWS_Soil.txt: columns 7-9 removed - typical-general.csv: OBS_SM* rows removed 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Addresses review feedback to make observed soil moisture feature fully usable: Documentation: - Add "Using Observed Soil Moisture" section to SUEWS_Soil.rst - Include complete YAML configuration example - Add commented soil_observation example to sample_config.yml Validation: - Add SMDMethod-soil_observation cross-check in phase_b.py - Clear error message when SMDMethod>0 but soil_observation missing Testing: - Add TestObservedSoilMoistureIntegration class with two tests - test_observed_soil_moisture_affects_evaporation: physics validation - test_smdmethod_config_propagates_to_kernel: pipeline validation - Add pytestmark = pytest.mark.core to test module Code fixes: - Use logger_supy instead of logging.getLogger in site.py - Simplify redundant condition check in SoilObservationConfig 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add note explaining that soil_observation applies to the entire site, not per land cover type. A soil moisture sensor measures at a single physical location, so the same conversion parameters are used across all land cover types within the grid. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
sunt05
force-pushed
the
sunt05/gh3-xsmd-input
branch
from
b8059a0 to
bfc9652
Compare
- Clarify smcap constraints for volumetric vs gravimetric measurements - Add debug logging when falling back to None for missing columns - Document defence-in-depth validation in _forcing.py - Improve integration test comments explaining surface fraction changes - Remove noisy PASS result when SMDMethod=0 in Phase B validation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Extract duplicate field-to-column mapping into class constant - Simplify SMDMethod check using truthy evaluation - Reuse computed smd_methods Series instead of redundant column access Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reintroduces observed soil moisture support by converting xsmd inputs before calling SUEWS. Adds soil observation metadata to the land-cover schema plus docs describing the required fields. Documents the change in the changelog and adds unit tests that exercise volumetric and gravimetric conversions. Testing: source .venv/bin/activate && PYTHONPATH=src pytest test/core/test_soil_obs_conversion.py