Use the EGI coefficient in irrigation probability - #249
Open
Renato L. de F. Cunha (renatolfc) wants to merge 3 commits into
Open
Use the EGI coefficient in irrigation probability#249Renato L. de F. Cunha (renatolfc) wants to merge 3 commits into
Renato L. de F. Cunha (renatolfc) wants to merge 3 commits into
Conversation
Copilot started reviewing on behalf of
Renato L. de F. Cunha (renatolfc)
August 22, 2026 18:46
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes an implementation defect in the compute_irrigation_probability op where the EGI coefficient was not being applied (NGI was used twice), which would silently bias irrigation probability outputs. Adds a focused regression test to ensure distinct NGI/EGI coefficients are wired into the model correctly.
Changes:
- Correct logistic regression coefficient vector to use
coef_egifor the EGI feature. - Add a regression unit test that captures the model coefficient vector and asserts NGI/EGI/LST coefficients are distinct as provided.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ops/compute_irrigation_probability/compute_irrigation_probability.py | Fixes coefficient wiring so EGI uses coef_egi instead of duplicating NGI. |
| ops/compute_irrigation_probability/test_compute_irrigation_probability.py | Adds regression coverage to prevent reintroducing the NGI/EGI coefficient mix-up. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+32
to
+33
| with pytest.raises(RuntimeError, match="stop after coefficient capture"): | ||
| CallbackBuilder(1.0, 2.0, 3.0, 4.0)()(landsat, ngi, egi, lst, cloud_mask) # type: ignore |
Copilot started reviewing on behalf of
Renato L. de F. Cunha (renatolfc)
August 22, 2026 19:02
View session
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
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.
The irrigation callback accepts separate NGI and EGI coefficients, but the fitted logistic model currently inserts the NGI value twice. That silently gives every probability calculation the wrong EGI weight.
This PR changes the duplicated entry to
coef_egi. A focused regression uses distinct values and checks the model vector directly.Addresses the implementation defect found while investigating #223.