-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Standardized Streamflow Index and Standardized Groundwater level Index #1877
base: main
Are you sure you want to change the base?
Conversation
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! |
Warning This Pull Request is coming from a fork and must be manually tagged |
It was exactly the MLE with saddle point estimators, good job |
|
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
-------- | ||
>>> from datetime import datetime | ||
>>> from xclim.indices import standardized_groundwater_index | ||
>>> ds = xr.open_dataset(path_to_gwl_file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would need a xclim-testdata
same in the tests, I just used false data
|
||
|
||
class StandardizedIndexes(ResamplingIndicator): | ||
"""Resampling but flexible inputs indicators.""" | ||
|
||
src_freq = ["D", "MS"] | ||
context = "hydro" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aulemahal any opposition to this reorginazation? Previously I defined it in _precip, but now it's also needed in _hydrology
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just want to make sure this PR is not impeding the use of PWM for genextreme and gamma.
Resampling frequency. A monthly or daily frequency is expected. Option `None` assumes that desired resampling | ||
has already been applied input dataset and will skip the resampling step. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resampling frequency. A monthly or daily frequency is expected. Option `None` assumes that desired resampling | |
has already been applied input dataset and will skip the resampling step. | |
Resampling frequency. A monthly or daily frequency is expected. Option `None` assumes that the desired resampling | |
has already been applied to the input dataset and will skip the resampling step. |
uses a deterministic function that does not involve any optimization. | ||
`PWM` should be used with a `lmoments3` distribution. | ||
fitkwargs : dict, optional | ||
Kwargs passed to ``xclim.indices.stats.fit`` used to impose values of certains parameters (`floc`, `fscale`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kwargs passed to ``xclim.indices.stats.fit`` used to impose values of certains parameters (`floc`, `fscale`). | |
Kwargs passed to ``xclim.indices.stats.fit`` used to impose values of certain parameters (`floc`, `fscale`). |
Resampling frequency. A monthly or daily frequency is expected. Option `None` assumes that desired resampling | ||
has already been applied input dataset and will skip the resampling step. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resampling frequency. A monthly or daily frequency is expected. Option `None` assumes that desired resampling | |
has already been applied input dataset and will skip the resampling step. | |
Resampling frequency. A monthly or daily frequency is expected. Option `None` assumes that the desired resampling | |
has already been applied to the input dataset and will skip the resampling step. |
The approximate method uses a deterministic function that does not involve any optimization. | ||
`PWM` should be used with a `lmoments3` distribution. | ||
fitkwargs : dict, optional | ||
Kwargs passed to ``xclim.indices.stats.fit`` used to impose values of certains parameters (`floc`, `fscale`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kwargs passed to ``xclim.indices.stats.fit`` used to impose values of certains parameters (`floc`, `fscale`). | |
Kwargs passed to ``xclim.indices.stats.fit`` used to impose values of certain parameters (`floc`, `fscale`). |
# "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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean PWM ? Why add genextreme and lognorm here ?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand what is checked here, beyond unit changes.
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: