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

Exchange xr.Dataset.dims by xr.Dataset.sizes #159

Merged
merged 4 commits into from
Mar 15, 2024

Conversation

christopher-lw
Copy link
Contributor

Change Summary

To eliminate FutureWarnings by the call of xr.Dataset.dims in xwrf it is replaced by xr.Dataset.sizes according to the warning:

FutureWarning: The return type of `Dataset.dims` will be changed to return a set of dimension names in future, in order to be more consistent with `DataArray.dims`. To access a mapping from dimension names to lengths, please use `Dataset.sizes`.

Related issue number

No issue opened

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable

Please add any other relevant info below:

  • No additional test were implemented so far. I am open for suggestions if these are required for a change of this scope.
  • Documentration change probably not needed

Copy link
Collaborator

@lpilz lpilz left a comment

Choose a reason for hiding this comment

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

Hi Christopher,

thanks a lot for your PR. This is indeed a problem, which we should fix, however I'm not completely sure how one is supposed to do this nicely.

You suggested changes to a couple of ds.dims calls, which are actually used as intended but still raise the warning. Currently, I see 4 possible remedies:

  1. We could either change these to ds.sizes calls which also works because it iterates over the keys.
  2. We could add a warnings.filterwarnings('ignore', module='xarray', category=FutureWarning, message="The return type of Dataset.dims*") to the __init__.py.
  3. We could add a context manager around the offending lines to suppress this specific warning.
  4. We could add a context manager inside of the user-facing API (i.e. inside the whole .postprocess and .destagger methods).
  5. We ignore the warnings by line number warnings.simplefilter('ignore', lineno=<no>)

My opinions:

  1. It's fast and clean but I don't like it because it is the intended use of ds.dims
  2. This works but it probably also changes the user's warnings environment.
  3. IMO this is a good solution from an engineering standpoint. It creates a lot of bloat-code though, which we will have to remove again.
  4. I'm not sure how smart that is. This makes huge changes in git-blame (because of the indenting) and does not suppress the warnings when testing individual functions.
  5. We will have to change the lineno every time we change something in the code but IMO this is the nicest solution, esp. because the code base does not change very quickly any more.

What do you think, @christopher-lw? And maybe @jthielen or @andersy005 know the best-practice solution for these kinds of issues?

Edit: I misunderstood the lineno argument. This is the line of where the warning is raised (i.e. in the xarray/core.py file) and the the line where it is triggered (i.e. in our code).

xwrf/accessors.py Outdated Show resolved Hide resolved
xwrf/postprocess.py Outdated Show resolved Hide resolved
xwrf/postprocess.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@lpilz lpilz left a comment

Choose a reason for hiding this comment

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

LGTM, sorry for my confusion.

@lpilz lpilz merged commit 3163abc into xarray-contrib:main Mar 15, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants