Standardized Streamflow Index and Standardized Groundwater level Index #1877
Conversation
LamAdr
changed the title
Co-authored-by: Trevor James Smith <[email protected]>
Co-authored-by: Trevor James Smith <[email protected]>
Co-authored-by: Trevor James Smith <[email protected]>
Co-authored-by: Trevor James Smith <[email protected]>
|
Where things stand
|
|
Just an FYI, but some tests are expected to fail until #1889 is merged! |
|
|
Ignore the Coveralls problem for now (I think their service is coming back online gradually). I've removed that as a required check for the time being. I think we could safely add For reviewing, I don't mind jumping in, but I think @huard would be better. |
for more information, see https://pre-commit.ci
| # "WPM" method doesn't seem to work for gamma or pearson3 | ||
| dist_and_methods = { | ||
| "gamma": ["ML", "APP"], | ||
| "fisk": ["ML", "APP"], | ||
| "genextreme": ["ML", "APP"], | ||
| "lognorm": ["ML", "APP"], | ||
| } |
There was a problem hiding this comment.
Do you mean PWM ? Why add genextreme and lognorm here ?
There was a problem hiding this comment.
genextreme is among the suggest choices for Standardized Streamflow index (SSI): https://agupubs.onlinelibrary.wiley.com/doi/10.1029/2019WR026315 . lognormal is suggested for SGI: https://hess.copernicus.org/articles/17/4769/2013/
For the method PWM, no restriction is placed on the choice of the distribution, that's why it doesn't appear here. You just have to supply a lmoments3 distribution. Since we don't support officially lmoments3, I thought we might as well not restrict the use of PWM with lmoments3, we give no insurance quality, contrary to the other distributions: those combinations of distribution/method above work with scipy and have been tested for standardized indices computations.
| out1 = land.standardized_streamflow_index( | ||
| q, | ||
| freq="MS", | ||
| window=1, | ||
| dist="genextreme", | ||
| method="APP", | ||
| fitkwargs={"floc": 0}, | ||
| ) | ||
| out2 = land.standardized_streamflow_index( | ||
| qMM, | ||
| freq="MS", | ||
| window=1, | ||
| dist="genextreme", | ||
| method="APP", | ||
| fitkwargs={"floc": 0}, | ||
| ) | ||
| np.testing.assert_array_almost_equal(out1, out2, 3) |
There was a problem hiding this comment.
Not sure I understand what is checked here, beyond unit changes.
There was a problem hiding this comment.
I didn't write the test, Adrien did. But looking at xclim code, this is how many test_3d_data_with_nans start. In this case, there is no nan at the end to check specifically if a nan is kept in place, because with the monthly resampling there is no more nan, so it becomes a bit dull indeed. I added another test with a daily computation with a nan insertion, and check that the SSI is a nan as expected.
The values of the output are already tested in test_indices
|
Sorry, I didn't see you had reviewed the PR @huard . I will address your comments now. |
Co-authored-by: David Huard <[email protected]>
On the contrary, PWM is allowed with any distribution found in lmoments3. It doesn't appear in the |
Co-authored-by: David Huard <[email protected]>
Pull Request Checklist:
number) and pull request (:pull:number) has been addedWhat kind of change does this PR introduce?
Does this PR introduce a breaking change?
No
Other information: