Conversation
for more information, see https://pre-commit.ci
…cator # Conflicts: # src/xclim/indicators/land/_streamflow.py
aulemahal
left a comment
There was a problem hiding this comment.
Sorry if I'm late and opiniated...
There was a problem hiding this comment.
To remove before merging!
| abstract = {The duration of the seasonal snowpack determines numerous aspects of the | ||
| water cycle, ecology and the economy in cold and mountainous regions, and is a balance | ||
| between the magnitude of accumulated snow and the rate of melt. The contribution of | ||
| each component has not been well quantified under contrasting topography and | ||
| climatological conditions although this may provide useful insights into how snow cover | ||
| duration could respond to climate change. Here, we examined the contribution of the | ||
| annual peak snow water equivalent (SWE) and the seasonal melt rate to define the | ||
| duration of the snowpack over temperate mountains, using snow data for mountain areas | ||
| with different climatological characteristics across the Iberian Peninsula. We used a | ||
| daily snowpack database for the period 1980--2014 over Iberia to derive the seasonal | ||
| peak SWE, melt rate and season snow cover duration. The influence of peak SWE and melt | ||
| rates on seasonal snow cover duration was estimated using a stepwise linear model | ||
| approach. The stepwise linear models showed high R-adjusted values (average R-adjusted | ||
| = 0.7), without any clear dependence on the elevation or geographical location. In | ||
| general, the peak SWE influenced the snow cover duration over all of the mountain areas | ||
| analysed to a greater extent than the melt rates (89.1\%, 89.2\%, 81.6\%, 93.2\% and | ||
| 95.5\% in the areas for the Cantabrian, Central, Iberian, Pyrenees and Sierra Nevada | ||
| mountain ranges, respectively). At these colder sites, the melt season occurs mostly in | ||
| the spring and tends to occur very fast. In contrast, the areas where the melt rates | ||
| dominated snow cover duration were located systematically at lower elevations, due to | ||
| the high interannual variability in the occurrence of annual peak SWE (in winter or | ||
| early spring), yielding highly variable melt rates. However, in colder sites the melt | ||
| season occurs mostly in spring and it is very fast in most of the years. The results | ||
| highlight the control that the seasonal precipitation patterns, in combination with | ||
| temperature, exert on the seasonal snow cover duration by influencing the peak SWE and | ||
| suggest a future increased importance of melt rates as temperatures increase. Despite | ||
| the high climatological variability of the Iberian mountain ranges, the results showed | ||
| a consistent behaviour along the different mountain ranges, indicating that the methods | ||
| and results may be transferrable to other temperate mountain areas of the world.} |
There was a problem hiding this comment.
Do we need the full abstract here ? For a smaller file and for coherence with the other entires, I think we could remove the abstracts here.
| days_with_snowpack = Snow( | ||
| title="Days with snowpack", | ||
| identifier="days_with_snowpack", | ||
| units="days", | ||
| long_name="Days with snowpack", | ||
| description="The total number of days with snow water equivalent (SWE) above a given threshold.", | ||
| compute=xci.days_with_snowpack, | ||
| ) |
There was a problem hiding this comment.
This already exists as snw_days_above, no ?
| @declare_units(swe="[length]", thresh="[length]") | ||
| def days_with_snowpack( | ||
| swe: xarray.DataArray, |
There was a problem hiding this comment.
Ah I see the nuance now. I would argue that swe is exactly the same as snw and the difference in variable doesn't justify a new indicator. Also, snw is the CMIP name, which is why we chose it in xclim.
There was a problem hiding this comment.
This makes me think of a PR I co-authored with Ludwig, I could dig this up.
There is the special field [precipitation], which can do automatic conversions. Having this with snw -> swe would be dangerous. What you say is that user should do all their work in snw, then convert to swe at the end, correct? snw_to_snd functions already exist, it would be easy to add a new converter with the correct density to have snw_to_swe
There was a problem hiding this comment.
In the context of xclim, what I think is that we should try to limit the number of non-conventional variables to a minimum. swe is so close to snw that I don't think it warrants a special treatment.
Also, days_with_snowpack and snw_days_above are two very different names for the same thing. The {var}_days_above construct being the choice made by xclim for clear and systematic naming, unless days_with_snowpack is a jargon term refering to a (domain-)specific algorithm.
Finally, if we really need to keep this indicator, I would suggest removing the compute function nonetheless and simply creating a day_with_snowpack in indicators/land/_snow.py instead.
But outside of the context of xclim, in my very personal opinion, I find talking about snow in terms of liquid water a bit weird. You don't need to melt it to study it on a computer.
There was a problem hiding this comment.
I forgot about the distinction and already removed it.
| @declare_units(swe="[length]", q="[discharge]") | ||
| def lag_snowpack_flow_peaks( | ||
| swe: xarray.DataArray, |
| docs/paper/tmp_saved_input_paper.bib | ||
| docs/tmp_saved_input_references.bib |
There was a problem hiding this comment.
| docs/paper/tmp_saved_input_paper.bib | |
| docs/tmp_saved_input_references.bib | |
| docs/tmp_saved_input_references.bib | |
| docs/paper/tmp_saved_input_paper.bib |
revert unneeded change
| @declare_units(swe="[length]", thresh="[length]") | ||
| def days_with_snowpack( | ||
| swe: xarray.DataArray, |
There was a problem hiding this comment.
This makes me think of a PR I co-authored with Ludwig, I could dig this up.
There is the special field [precipitation], which can do automatic conversions. Having this with snw -> swe would be dangerous. What you say is that user should do all their work in snw, then convert to swe at the end, correct? snw_to_snd functions already exist, it would be easy to add a new converter with the correct density to have snw_to_swe
Signed-off-by: Trevor James Smith <10819524+Zeitsperre@users.noreply.github.com>
for more information, see https://pre-commit.ci
|
@coxipi I performed a merge of I may have totally botched the merge, so apologies. |
|
Anyways, we can discuss what to do with this in the next meeting. I'm not sure what your botched merge entails, but we'll figure something out |
Nope, just the last merge commit I performed this morning. I have some time to see if I can clean it up before Miller Time. Update: Managed to clean up the mess. Testing is failing as |
Signed-off-by: Trevor James Smith <10819524+Zeitsperre@users.noreply.github.com>
Signed-off-by: Trevor James Smith <10819524+Zeitsperre@users.noreply.github.com>
…xclim into hydro_indicator
| v0.60.0 (2026-01-23) | ||
| -------------------- | ||
| Contributors to this version: Éric Dupuis (:user:`coxipi`), Trevor James Smith (:user:`Zeitsperre`), Juliette Lavoie (:user:`juliettelavoie`), Ève Larose (:user:`e-larose`), Faisal Mahmood (:user:`faimahsho`), David Huard (:user:`huard`), Pascal Bourgault (:user:`aulemahal`). | ||
| Contributors to this version: Éric Dupuis (:user:`coxipi`), Trevor James Smith (:user:`Zeitsperre`), Juliette Lavoie (:user:`juliettelavoie`), Faisal Mahmood (:user:`faimahsho`), David Huard (:user:`huard`), Pascal Bourgault (:user:`aulemahal`). |
There was a problem hiding this comment.
Ève's other PR made it into the last version ;)
| Contributors to this version: Éric Dupuis (:user:`coxipi`), Trevor James Smith (:user:`Zeitsperre`), Juliette Lavoie (:user:`juliettelavoie`), Faisal Mahmood (:user:`faimahsho`), David Huard (:user:`huard`), Pascal Bourgault (:user:`aulemahal`). | |
| Contributors to this version: Éric Dupuis (:user:`coxipi`), Trevor James Smith (:user:`Zeitsperre`), Juliette Lavoie (:user:`juliettelavoie`), Ève Larose (:user:`e-larose`), Faisal Mahmood (:user:`faimahsho`), David Huard (:user:`huard`), Pascal Bourgault (:user:`aulemahal`). |
There was a problem hiding this comment.
Don't forget to remove the print calls!
|
|
||
| import numpy as np | ||
| import pytest | ||
| import xarray as xr |
There was a problem hiding this comment.
| import xarray as xr | |
| from xarray import DataArray |
| out = land.days_with_snowpack(swe, freq="YS") | ||
|
|
||
| assert out.attrs["units"] == "days" | ||
| assert isinstance(out, xr.DataArray) |
There was a problem hiding this comment.
| assert isinstance(out, xr.DataArray) | |
| assert isinstance(out, DataArray) |
Pull Request Checklist:
number) and pull request (:pull:number) has been addedWhat kind of change does this PR introduce?
Adds Hydrological signature indicator wrapper
Does this PR introduce a breaking change?
Other information: