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 q (streamflow) to variables.yml #1996

Merged
merged 8 commits into from
Nov 22, 2024
Merged

Add q (streamflow) to variables.yml #1996

merged 8 commits into from
Nov 22, 2024

Conversation

coxipi
Copy link
Contributor

@coxipi coxipi commented Nov 13, 2024

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?

  • Add q as a proper variable in variables.yml

Does this PR introduce a breaking change?

No

Other information:

Discussing with @RondeauG , we were deliberating between qstr, qstrm, strf. I haven't seen any of these variables in many places in litterature, there doesn't seem to be a standard abbreviation apart from Q or q (flow or specific flow ), so I'm enclined to change strf if it doesn't feel right.

@github-actions github-actions bot added the indicators Climate indices and indicators label Nov 13, 2024
@Zeitsperre Zeitsperre requested a review from RondeauG November 13, 2024 18:46
@Zeitsperre Zeitsperre added this to the v0.54.0 milestone Nov 13, 2024
@aulemahal
Copy link
Collaborator

But if q is the most common standard, why would we not use that ?

@coxipi
Copy link
Contributor Author

coxipi commented Nov 13, 2024

But if q is the most common standard, why would we not use that ?

I just thought that the variable associated with the streamflow should have its standard naming declared with units in variables.yml. Then, if we define q as a streamflow in variables.yml, I'm not sure it's a good idea to use it as a variable for quantiles, no?

@aulemahal
Copy link
Collaborator

Most references to q as "quantile" I see in xclim are in sdba, where I would assume context is enough to make the difference ? There are no indicators that refer to quantiles as q, only xc.indices.stats.parametric_quantile does it and I would argue again that context would be enough here.

I think my point is that such a breaking change doesn't seem justified if it is not based on convention.

@coxipi coxipi changed the title streamflow uses strf instead of q as a variable Add q (streamflow) to variables.yml Nov 14, 2024
@coxipi
Copy link
Contributor Author

coxipi commented Nov 14, 2024

Ok, I've just replaced the streamflow entry in variables.yml with q then.

Copy link
Contributor

@RondeauG RondeauG left a comment

Choose a reason for hiding this comment

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

Sorry, I didn't see that you had requested a review from me!

@Zeitsperre Zeitsperre added the approved Approved for additional tests label Nov 22, 2024
@coxipi coxipi merged commit 75c0d11 into main Nov 22, 2024
21 checks passed
@coxipi coxipi deleted the strf_hydrovariable branch November 22, 2024 17:31
@coxipi coxipi mentioned this pull request Dec 10, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved for additional tests indicators Climate indices and indicators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants