Skip to content

Read the maximum leaf area index from a land use lookup table - #354

Merged
soaressgabriel merged 14 commits into
mainfrom
feat_lai_max
Sep 22, 2026
Merged

soaressgabriel merged 14 commits into
mainfrom
feat_lai_max

Conversation

@soaressgabriel

Copy link
Copy Markdown
Collaborator

Checklist

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Added tests for changed code.
  • Updated documentation for changed code.

Description

Adds an optional lookup table with the maximum leaf area index of each land use class, TABLES.lai_max (format 1.0: lookup_tables.lai_max), selected by the constant CONSTANTS.lai_max_from_table (format 1.0: model_constants.lai_max_from_table, default false). With the switch on, the model reads LAI_max per cell with lookupscalar on the land use raster of the step, next to the other land use tables; with it off, the lai_max constant is used as before, so existing configurations and the golden fixtures are unchanged.

The first commit is the implementation by @LINAMARIAOSORIO (subject translated to English); the commits on top answer the review of the branch and the gaps found while doing so:

  • Switch without a table. The loader reports lai_max_from_table: true without TABLES.lai_max as a blocking problem (ConfigurationError) before the run, in both formats and also when the input validation is skipped with -s, instead of failing inside lookupscalar at the first step. A table given while the switch is off is reported as ignored (non-blocking), not dropped silently; it must exist, like every declared input, but its content is not checked.
  • Optional table normalization. InputTableFiles keeps the _normalise validator of the required tables as on main and gives the optional table its own validator, as InputRasterFiles does for the optional rasters: None, "" and b"" mean not specified, and a path is normalized with as_path. On the branch an empty required path was not skipped silently: it ended as a pydantic type error (Input should be a valid string) instead of the FileNotFoundError of main; it is a FileNotFoundError again. Both file models also read an empty lai_max as not specified before the paths are anchored, so an empty string no longer becomes the configuration directory.
  • Canonical legacy dump. ModelConfigurationFile.to_dict() now carries TABLES.lai_max: null when the table is not given, as it already does for the optional rasters; the two round-trip tests that compared the dump with the input were updated for it (they were the two failures on the untouched branch). The format 1.0 document leaves the key out.
  • Content validation. check_lookup_tables checks the table when the run reads it: readable, positive values at most the admissible maximum of the lai_max constant (12, from the application settings) and, one row per class, the land use classes of the vegetated area fraction table (which the area fraction checks already force to agree), so a class missing from the table blocks before the run instead of giving a missing value in the LAI and in the interception. An interval key is not matched against the classes, as in the other key checks of the module.
  • Format 1.0 loader. __build_from_v1 passed neither lai_max_from_table nor lookup_tables.lai_max to the model settings: the format 1.0 file model accepted the keys but the loader ignored them. Both are wired, and the legacy and format 1.0 files round-trip the keys (rubem config migrate carries them over, relative paths are anchored).
  • Documentation build. The annotation float | Field in _interception.py is evaluated at definition time and fails sphinx-build -W where PCRaster is mocked; the module defers its annotations, as the modules that combine a PCRaster type with | in an annotation already do.
  • Style. Ruff format and check at the pinned version, comments in Portuguese and the # ==== separators removed, the interception block back to its shape on main with the lookup placed with the other land use attributes, __str__ and docstrings of the settings models updated.
  • Tests and documentation. See below; the user guide, the file format reference, the model overview and the changelog describe the table.

Related Issue

Motivation and context

The interception module scaled the leaf area index of every cell with one basin-wide lai_max, whatever the land use class, while every other land use parameter of the model is already a per-class lookup table. The published formulation defines LAI_max as a single constant, so the table is an opt-in extension and the model overview states it as such.

Two points for the reviewer:

  • The user guide states 1 <= LAI_max <= 12 for the constant while the code accepts 0 < lai_max <= 12 (the application settings range and the positivity check). The table is validated with the range the code enforces; the discrepancy in the documentation of the constant predates this change and is left as it is.
  • The constant lai_max stays mandatory and validated even when the table is used; the switch only selects where the maximum comes from.

How has this been tested

  • pytest --ignore=tests/integration/doc -n auto on Python 3.13 with PCRaster and GDAL: 1316 passed, 1 skipped. On the untouched branch two tests of the legacy file model failed (the canonical TABLES dump now carries lai_max).
  • New tests: the optional table setting (empty means not specified, path normalization, existence and emptiness, the required tables still refuse an empty path); the legacy and format 1.0 file models (defaults, empty string, string switch, canonical dump, path anchoring, round trip, JSON schema); the loader in both formats (switch without a table blocks with and without -s, table without the switch warns and its content is not checked, the settings reach the model); the content checks (non-positive, above 12, missing and extra classes, interval key, unreadable, key spellings, skipped when the switch is off); the interception with a constant and with a field (a characterization test: the annotation fix is covered by the documentation build); and in-process runs on the synthetic dataset: a table whose values equal the constant reproduces the constant run exactly (every raster and time series, zero tolerance); with the table {3: 9, 4: 4} the first step (class 3) reproduces every output of a run with the constant 9 and the interception of the second step (class 4) the one of a run with the constant 4, both differing from the run with the constant 12; and the run log states the source of the maximum.
  • ruff format --check, ruff check (0.16.4) and codespell (2.4.3): clean.
  • sphinx-build -W -b html doc/source in a pip-only Python 3.13 environment with doc/requirements.txt, as the CI docs job: clean. On the untouched branch it failed with TypeError: unsupported operand type(s) for |: 'type' and 'Field'.

Screenshots

  • N/A

LINAMARIAOSORIO and others added 14 commits September 22, 2026 08:52
Move the lookup next to the other land use tables of the step and keep the
interception block as on main, with the maximum passed in as a constant or
a field. Defer the annotations of the interception module: the union
`float | Field` was evaluated at definition time, which fails the
documentation build where PCRaster is mocked.
The optional table gets its own before-validator, as the optional rasters
have: an empty setting means not specified and a path is normalised like
the other tables, while the required tables keep refusing an empty path.
The format 1.0 loader passes `lai_max_from_table` and `lookup_tables.lai_max`
to the model settings, as the legacy loader does. The switch without a table
is a blocking problem and a table without the switch is reported as ignored,
in both formats and whether or not the input validation is skipped.
When the table is given it must be readable, positive and at most the
admissible maximum of the `lai_max` constant, and keyed by the land use
classes of the vegetated area fraction table: a class absent from the table
gives a missing value in the LAI and in the interception.
Cover the optional table setting (empty means not specified, path
normalisation, existence and emptiness), the legacy and format 1.0 files
(defaults, empty string, string switch, canonical dump, path anchoring,
round trip, JSON schema), the loader in both formats (switch without a
table blocks, table without the switch warns, settings reach the model),
the content checks, the interception with a constant and with a field, and
two in-process runs on the synthetic dataset: a table equal to the constant
reproduces the constant run exactly, a table with a maximum per class
changes the interception at every step.
Describe the table and its switch in the user guide, add the file format
reference of the table, list it among the tables the land use raster must
match, and note in the model overview that reading the maximum per land use
class is an opt-in extension of the published formulation.
Log once, at INFO and before the first step, whether the maximum leaf area
index comes from the constant or from the land use table and which table,
as the initial step does for the LDD, so the log of a finished run records
the mode used.
The legacy file already reads an empty `TABLES.lai_max` as not specified;
in the format 1.0 file the empty string survived to `resolve_paths`, which
anchored it on the base directory and handed a directory path to the
lookup table settings.
The legacy loader and the interception block keep the spacing they have
on main; only the lines that change remain in the diff.
With the switch off a declared table must exist, like every table given,
but its content plays no part in the run: the loader reported it as
ignored and, at the same time, could block the run on its values. The
content checks now run only when the switch is on, and both problem
messages name the keys of the two configuration formats. The loader tests
of the switch and the table cover both formats.
The run with the table {3: 9, 4: 4} must reproduce every output of the
first step of a run with the constant 9 and the interception of the second
step of a run with the constant 4, not merely differ from the constant
run. The run-level tests get their own class, and the path normalisation
test feeds a spelling only as_path collapses.
The table section sits beside the constant, the constant stays mandatory
and validated when the table is read, the table must list one row per
class, the overview names the table key, and the changelog states what
the -s option skips.
@codecov

codecov Bot commented Sep 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.87500% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.99%. Comparing base (9721f1c) to head (58e904f).

Files with missing lines Patch % Lines
rubem/validation/lookup_tables.py 91.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #354      +/-   ##
==========================================
+ Coverage   92.93%   92.99%   +0.05%     
==========================================
  Files          63       63              
  Lines        4163     4223      +60     
  Branches      532      543      +11     
==========================================
+ Hits         3869     3927      +58     
- Misses        235      237       +2     
  Partials       59       59              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@soaressgabriel
soaressgabriel merged commit 5165534 into main Sep 22, 2026
17 checks passed
@soaressgabriel
soaressgabriel deleted the feat_lai_max branch September 22, 2026 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Read the maximum leaf area index (LAI_max) from a land use lookup table

3 participants