Skip to content
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

Add Kling-Gupta Efficiency #172

Merged
merged 11 commits into from Feb 8, 2022
Merged

Add Kling-Gupta Efficiency #172

merged 11 commits into from Feb 8, 2022

Conversation

jarq6c
Copy link
Collaborator

@jarq6c jarq6c commented Feb 7, 2022

This PR adds the Kling-Gupta Efficiency hydrological model calibration/evaluation metric.

Gupta, H. V., Kling, H., Yilmaz, K. K., & Martinez, G. F. (2009). Decomposition of the mean squared error and NSE performance criteria: Implications for improving hydrological modelling. Journal of hydrology, 377(1-2), 80-91.

Additions

  • hydrotools.metrics.metrics.kling_gupta_efficiency
  • hydrotools.metrics._validation

Removals

  • None

Changes

  • None

Testing

  1. Added test_kling_gupta_efficiency to test_metrics
  2. Added test_validation to check input validation methods

Notes

Todos

  • Implement later refinements to this metric, as needed.

Checklist

  • PR has an informative and human-readable title
  • PR is well outlined and documented. See #12 for an example
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (see CONTRIBUTING.md)
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output) using numpy docstring formatting
  • Placeholder code is flagged / future todos are captured in comments
  • Reviewers requested with the Reviewers tool ➡️

@jarq6c jarq6c added the enhancement New feature or request label Feb 7, 2022
@jarq6c jarq6c self-assigned this Feb 7, 2022
Copy link

@christophertubbs christophertubbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not only does it look good but it showed an error in some of my other code. And... approved.

@@ -1 +1 @@
__version__ = "1.1.3"
__version__ = "1.2.3"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldnt the version bump be to 1.2.0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Thanks!

@jarq6c
Copy link
Collaborator Author

jarq6c commented Feb 7, 2022

Not only does it look good but it showed an error in some of my other code. And... approved.

Thanks for the review!

a_scale: float = 1.0,
b_scale: float = 1.0
) -> float:
"""Compute the Kling-Gupta model efficiency coefficient (KGE).
Copy link
Member

@aaraney aaraney Feb 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should any software assumptions be stated here? Something like: "This function assumes two sorted arrays of equal length." I would also want to add something that conveys that y_true[0] "lines up with" y_pred[0]. I cant find an appropriate phrase though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth mentioning what the shape of the array's are assumed to be. i.e. this method should only be applied to 1 dimensional arrays.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This made this realize, the docstring description for these doesn't make any sense. I copied these from sklearn and forgot to update them for this case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated docstrings and added new input array validation methods for NSE and KGE. These could certainly be way more rigorous and exhaustive, but it's a start.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: the parameters descriptions explicitly state the expected shape ((n_samples,)).


References
----------
Gupta, H. V., Kling, H., Yilmaz, K. K., & Martinez, G. F. (2009). Decomposition of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@aaraney aaraney Feb 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on a side note, it would be "cool" if you could get the citation for the paper in bibtex or some other format. I am thinking a decorator that wraps each metric function and contains the bibtex formatted citation. You could access it like:

from hydrotools.metrics import metrics

print(metrics.kling_gupta_efficiency.citation(format="bibtex"))

@article{Gupta2009,
  doi = {10.1016/j.jhydrol.2009.08.003},
  url = {https://doi.org/10.1016/j.jhydrol.2009.08.003},
  year = {2009},
  month = oct,
  publisher = {Elsevier {BV}},
  volume = {377},
  number = {1-2},
  pages = {80--91},
  author = {Hoshin V. Gupta and Harald Kling and Koray K. Yilmaz and Guillermo F. Martinez},
  title = {Decomposition of the mean squared error and {NSE} performance criteria: Implications for improving hydrological modelling},
  journal = {Journal of Hydrology}
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be cool. Let's discuss that at next hydrotools meeting!

Copy link
Member

@hellkite500 hellkite500 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note about validation and error messaging to consider...

for x in arrays:
if len(np.array(x).shape) != 1:
message = "all input arrays must be 1-dimensional"
raise ValueError(message)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In both these validation functions, the exception may be too generic. It would be quite valuable to a developer if your messaging could indicate which array's fail validation -- or at the very least indicate what the differences are...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll note that these general _validation functions don't need to have this functionality, you can customize your ValueError exception so you can handle that higher up:

try:
   validate(...)
catch ShapeValueError:
    throw ValueError("Shape of y_true != y_pred: y_pred shape is {}, y_true shape is {}".format(y_pred.shape, y_true.shape)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated with new NonVectorError and InconsistentShapesError derived from Exception that report the offending arrays.

@jarq6c jarq6c merged commit 7294aac into NOAA-OWP:main Feb 8, 2022
@jarq6c jarq6c deleted the add-kge branch February 8, 2022 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Kling-Gupta
4 participants