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

Create _streamflow_flow_indices.py #1832

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

faimahsho
Copy link

streamflow_flow_indice is a submodule in the xclim indices module to calculate streamflow signatures, representing individual watershed characteristics of large river/lake basins.

Pull Request Checklist:

  • This PR addresses an already opened issue (for bug fixes / features)
    • This PR fixes #xyz
  • Tests for the changes have been added (for bug fixes / features)
    • (If applicable) Documentation has been added / updated (for bug fixes / features)
  • CHANGELOG.rst has been updated (with summary of main changes)
    • Link to issue (:issue:number) and pull request (:pull:number) has been added

What kind of change does this PR introduce?

  • ...This PR introduces three hydrological signatures in xclim indices module. The functions included are to calculate the high and low flow variables of a data series of watersheds and generate a characteristic value of the catchment from the calculated hydrological signatures.

Does this PR introduce a breaking change?

This can develop unique watershed attributes.

Other information:

streamflow_flow_indice is a submodule in the xclim indices module to calculate streamflow signatures, representing individual watershed characteristics of large river/lake basins.
@github-actions github-actions bot added docs Improvements to documenation indicators Climate indices and indicators labels Jul 8, 2024
Copy link

github-actions bot commented Jul 8, 2024

Welcome, new contributor!

It appears that this is your first Pull Request. To give credit where it's due, we ask that you add your information to the AUTHORS.rst and .zenodo.json:

  • The relevant author information has been added to AUTHORS.rst and .zenodo.json

Please make sure you've read our contributing guide. We look forward to reviewing your Pull Request shortly ✨

@tlogan2000 tlogan2000 requested a review from huard July 9, 2024 14:23
Copy link

github-actions bot commented Jul 9, 2024

Warning
This Pull Request is coming from a fork and must be manually tagged approved in order to perform additional testing.

Copy link
Collaborator

@huard huard left a comment

Choose a reason for hiding this comment

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

If the flow_index is not in the literature, I suggest to remove it from this PR and keep it in your project scripts.

Once this PR is merged in, I think it would be interesting to look at additional indices that requires other variables like precipitation, as I think it would provide better physical understanding of processes.

The next step will be to write a test for each index to make sure it behaves as expected. You'll see plenty of examples in the test directory.

xclim/indices/_streamflow_flow_indices.py Outdated Show resolved Hide resolved
xclim/indices/_streamflow_flow_indices.py Outdated Show resolved Hide resolved
xclim/indices/_streamflow_flow_indices.py Outdated Show resolved Hide resolved
xclim/indices/_streamflow_flow_indices.py Outdated Show resolved Hide resolved
xclim/indices/_streamflow_flow_indices.py Outdated Show resolved Hide resolved
xclim/indices/_streamflow_flow_indices.py Outdated Show resolved Hide resolved
xclim/indices/_streamflow_flow_indices.py Outdated Show resolved Hide resolved
xclim/indices/_streamflow_flow_indices.py Outdated Show resolved Hide resolved
xclim/indices/_streamflow_flow_indices.py Outdated Show resolved Hide resolved
xclim/indices/_streamflow_flow_indices.py Outdated Show resolved Hide resolved
.DS_Store Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this file and add .DS_Store to the .gitignore. I'm surprised it wasn't already there!

Copy link
Collaborator

Choose a reason for hiding this comment

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

This file still needs to be removed, then I would add this file to the bottom of the gitignore:

# Apple
.DS_Store

xclim/indices/_streamflow_flow_indices.py Outdated Show resolved Hide resolved
xclim/indices/_streamflow_flow_indices.py Outdated Show resolved Hide resolved
docs/references.bib Outdated Show resolved Hide resolved
@faimahsho faimahsho force-pushed the Watershed-indices branch 2 times, most recently from 948fa30 to 0507d4c Compare July 16, 2024 21:01
@@ -2152,3 +2152,35 @@ @article{droogers2002
url = {https://www.scopus.com/inward/record.uri?eid=2-s2.0-0036464359&doi=10.1023%2fA%3a1015508322413&partnerID=40&md5=7322aaa4c6874878f5b1dab3c73c1718},
type = {Article}
}

@article{addor2018,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good ! Now please use cite commands in the docstrings to refer to those. You'll see an example of how it's done in indices/_synoptic.py.

For example,

:cite:p:`addor2018`

to add a reference in parenthesis in the text (use :cite:t: for textual citations without parentheses), and

:cite:cts:`addor2018`

to add the citation in the References section.

Copy link
Collaborator

@huard huard left a comment

Choose a reason for hiding this comment

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

Looks good. Please review the changes I just pushed, and add tests for the "indicators" I created.

Note that I added an indexer argument, which lets you pick a season or specific months. Let me know if you believe this is useful or not.

I think the next step would be to have Louise review the finalized PR.

xclim/data/fr.json Outdated Show resolved Hide resolved
xclim/indices/_hydrology.py Outdated Show resolved Hide resolved
xclim/indices/_hydrology.py Outdated Show resolved Hide resolved
xclim/indices/_hydrology.py Outdated Show resolved Hide resolved
xclim/indices/_streamflow_flow_indices.py Outdated Show resolved Hide resolved
@faimahsho faimahsho force-pushed the Watershed-indices branch 3 times, most recently from d8ae74d to 7beffe9 Compare July 18, 2024 15:36
…ean over whole period. This part of the analysis can be done on the user-side. Added support for **indexer, allowing more freedom when selecting the period
xclim/indices/_hydrology.py Outdated Show resolved Hide resolved
xclim/indices/_hydrology.py Outdated Show resolved Hide resolved
faimahsho and others added 3 commits July 26, 2024 09:28
Co-authored-by: Trevor James Smith <10819524+Zeitsperre@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements to documenation indicators Climate indices and indicators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants