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

frequency_analysis indicator weirdly declared #1130

Closed
2 tasks done
aulemahal opened this issue Jul 14, 2022 · 1 comment · Fixed by #1225
Closed
2 tasks done

frequency_analysis indicator weirdly declared #1130

aulemahal opened this issue Jul 14, 2022 · 1 comment · Fixed by #1225
Assignees
Labels
bug Something isn't working InfoCrue
Milestone

Comments

@aulemahal
Copy link
Collaborator

Setup Information

  • Xclim version: master
  • Python version: all
  • Operating System: all

Description

freq_analysis = FA(
identifier="freq_analysis",
var_name="q{window}{mode:r}{indexer}",
long_name="N-year return period {mode} {indexer} {window}-day flow",
description="Streamflow frequency analysis for the {mode} {indexer} {window}-day flow "
"estimated using the {dist} distribution.",
units="m^3 s-1",
title="Flow values for given return periods.",
compute=declare_units(da=None)(frequency_analysis),
)

Here you see that the output of frequency_analysis is a discharge (var name q, units m³/s). However, the input variable has a generic name da and no prescribed dimensionality. This would mean that if you pass anything else then a discharge, the unit check will not fail, but the indicator will fail when attempting to convert the output units.

Indeed, the next example fails. But even weirder, is that we have a special CF check that looks for the "water_volume_transport_in_river_channel" standard name. Thus, we already are expecting a discharge.

Steps To Reproduce

import xclim as xc
from xclim.testing import open_dataset

ds = open_dataset('ERA5/daily_surface_cancities_1990-1993.nc')
xc.land.freq_analysis(da=ds.dtas, t=100, dist='norm', mode='max')

warns:

UserWarning: Variable has a non-conforming standard_name: Got `dew_point_temperature`, expected `['water_volume_transport_in_river_channel']`

and fails:

DimensionalityError: Cannot convert from 'kelvin' ([temperature]) to 'meter ** 3 / second' ([length] ** 3 / [time])

The expect error would be:

ValidationError: Data units K are not compatible with requested [discharge].

Additional context

@RondeauG

Contribution

  • I would be willing/able to open a Pull Request to address this bug.

Code of Conduct

  • I agree to follow this project's Code of Conduct
@aulemahal aulemahal added the bug Something isn't working label Jul 14, 2022
@aulemahal aulemahal added this to the v0.38 milestone Jul 14, 2022
@Zeitsperre Zeitsperre modified the milestones: v0.38, v0.39 Sep 6, 2022
@RondeauG
Copy link
Contributor

Adding to this issue, I think a discharge variable should be added to VARIABLES if we want to add and support more and more discharge-related indicators (which I think is the case?).

As for the name, I'd argue that q is not good, since this is already used for "specific discharge" in hydrology, with units in m3 s-1 km-2. Q is used for discharge in m3 s-1, but I don't think that we want a capital letter?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working InfoCrue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants