Conversation
CI Build PlanChanged FilesPython source (2 files)
Tests (1 file)
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. |
|
hello @MatthewPaskin - this is now ready for you to have a look/test if it is working as expected! |
|
hi @MatthewPaskin - let me know what do you think about this once you have time :) |
|
@dayantur thanks, this looks good for grass. Given that the new lai options that I have worked on are not finalised/set yet I think it is fine that the code just checks if the surface is grass. We can update this later after the options have been finished. |
…alb_id-of-vegetated-surfaces
|
Hi @sunt05 - could you please have a look at this once you have time? Many thanks! |
sunt05
left a comment
There was a problem hiding this comment.
Thanks for adding this, @dayantur — the seasonal albedo logic is a natural extension of the existing LAI adjustment and the physical reasoning (green grass = lower albedo, full canopy = higher bulk albedo) is sound.
I've left a few comments below. The main one is a bug in _set_alb_id where values aren't persisted if the surface key is missing from initial_states — the existing LAI code has a guard for this that should be mirrored here. The rest are minor style/convention items.
|
hi @sunt05 - many thanks for the review :) changes should be now done! |
sunt05
left a comment
There was a problem hiding this comment.
Thanks for this, @dayantur -- the seasonal albedo logic is a nice complement to the existing LAI adjustment, and the test structure is clear.
Two things I'd like to see addressed before merging, and a few smaller suggestions.
Main concerns
The float comparison in _set_alb_id uses exact == to skip no-op adjustments. Because 0.5 * (0.10 + 0.20) evaluates to 0.15000000000000002 (not 0.15), the guard never triggers for midseason, producing spurious adjustment entries. The midseason test passes coincidentally because of this imprecision. A tolerance-based comparison (e.g. math.isclose) would make the behaviour predictable.
The new albedo adjustment doesn't check sfr > 0, unlike the LAI adjustment directly above it. If a vegetated surface has zero fraction, adjusting its albedo is physically meaningless and inconsistent with the existing pattern. Worth adding the same guard, or documenting why it's intentionally omitted.
Smaller items
The _get_range_and_id helper reimplements the dict-or-value extraction that get_value_safe already provides -- three calls to get_value_safe would replace ~23 lines. Both nested functions are also redefined on every site iteration since they're inside the loop; moving them above the loop (with explicit parameters) would be cleaner. Finally, test coverage for tropical/equatorial seasons, Southern Hemisphere, and missing alb_min/alb_max would strengthen confidence in the edge cases.
|
hi @sunt05 - will do the required changes tonight so we can get this in as well :) thanks! |
…alb_id-of-vegetated-surfaces
|
hi @sunt05 - I should have addressed all the suggested changes. Thanks for the second round of suggestions! Could you have a look again when you have time? Many thanks :) |
|
Thanks for this, @dayantur — the seasonal albedo logic is a clean extension of the existing LAI adjustment pattern, and you've addressed all the review comments thoroughly. I'll sort out the remaining minor bits (blank lines between the new helpers, etc.) and merge. |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Following the discussion in PR #1135, this PR introduces a new seasonal adjustment for vegetated surface albedo (
alb_id) based on the detected season.The implemented rules are as follows:
Summer
alb_id(grass)→ set toalb_minalb_id(dectr)→ set toalb_maxalb_id(evetr)→ set toalb_maxWinter
alb_id(grass)→ set toalb_maxalb_id(dectr)→ set toalb_minalb_id(evetr)→ set toalb_minOther Seasons (Spring / Autumn)
alb_idis set to the midpoint betweenalb_minandalb_max, i.e.for all vegetated surfaces.
In the future, we will make this update conditional to
laitypemodel option, currently under development by @MatthewPaskin!Main Changes