Skip to content

Point users to where in their code they should make mods for Dataset.dims - #8534

Merged
max-sixty merged 2 commits into
pydata:mainfrom
hmaarrfk:patch-4
Dec 10, 2023
Merged

Point users to where in their code they should make mods for Dataset.dims#8534
max-sixty merged 2 commits into
pydata:mainfrom
hmaarrfk:patch-4

Conversation

@hmaarrfk

Copy link
Copy Markdown
Contributor

Its somewhat annoying to get warnings that point to a line within a library where the warning is issued. It really makes it unclear what one needs to change.

This points to the user's access of the dims attribute.

  • Closes #xxxx
  • Tests added
  • User visible changes (including notable bug fixes) are documented in whats-new.rst
  • New functions/methods are listed in api.rst

…dims
Its somewhat annoying to get warnings that point to a line within a library where the warning is issued. It really makes it unclear what one needs to change.
This points to the user's access of the `dims` attribute.
Comment threadxarray/core/utils.py Outdated
"in order to be more consistent with `DataArray.dims`. To access a mapping from dimension names to lengths, "
"please use `Dataset.sizes`.",
FutureWarning,
stacklevel=3,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

mind switching it to

defemit_user_level_warning(message, category=None):
please?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

sure. i mean, i feel like stack inspection is so overkill though. the call the warning is mean to be fast. any kind of stack inspection in my mind is going to be slower.

The other advantage of emitting stacklevel=3 is that you can actually find internal instances of this within xarray.

However, as you wish. Please feel free to modify this as you see fit and merge, it is really meant to be a simple one liner and merged with ease.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes, fair — it is definitely a perf tradeoff.

"Ideally" the warnings aren't firing in hot paths / folks rework their code to prevent them. But it is a bit fuzzy.

If we can find a case where it doesn't work well, we could def change the policy...

@max-sixty

Copy link
Copy Markdown
Collaborator

Am going to force merge because the errors are unrelated. But they don't seem transient — I think there's an issue with the dependencies unfortunately.

(I'm guessing moving to a poetry-based installation would be too much of a lift? Upgrading dependencies deliberately with something like dependabot would eliminate 95%+ of our failures on main...)

@max-sixty
max-sixty merged commit 8d168db into pydata:mainDec 10, 2023
@hmaarrfk

Copy link
Copy Markdown
ContributorAuthor

Also, i'm not sure if you care, but:

$ grep \\\.warn\( . -R
./xarray/tests/test_assertions.py: warnings.warn("warning in test")
./xarray/tests/test_assertions.py: warnings.warn("warning in test")
./xarray/tests/test_assertions.py: warnings.warn("test")
./xarray/core/dataset.py: warnings.warn(
./xarray/core/dataset.py: warnings.warn(
./xarray/core/dataset.py: warnings.warn(
./xarray/core/dataset.py: warnings.warn(
./xarray/core/dataset.py: warnings.warn(
./xarray/core/dataset.py: warnings.warn(
./xarray/core/dataset.py: warnings.warn(
./xarray/core/dataset.py: warnings.warn(
./xarray/core/dataset.py: warnings.warn(
./xarray/core/dataset.py: warnings.warn(
./xarray/core/resample.py: warnings.warn(
./xarray/core/resample.py: warnings.warn(
./xarray/core/common.py: warnings.warn(
./xarray/core/common.py: warnings.warn(
./xarray/core/common.py: warnings.warn(
./xarray/core/accessor_dt.py: warnings.warn(
./xarray/core/computation.py: warnings.warn(
./xarray/core/computation.py: warnings.warn(
./xarray/core/groupby.py: warnings.warn(
./xarray/core/groupby.py: warnings.warn(
./xarray/core/dataarray.py: warnings.warn(
./xarray/core/dataarray.py: warnings.warn(
./xarray/core/options.py: warnings.warn(
./xarray/core/rolling.py: warnings.warn(
./xarray/core/variable.py: warnings.warn(
./xarray/core/variable.py: warnings.warn(
./xarray/core/variable.py: warnings.warn(
./xarray/core/variable.py: warnings.warn(
./xarray/core/nputils.py: warnings.warn("Polyfit may be poorly conditioned", RankWarning, stacklevel=2)
./xarray/core/duck_array_ops.py: warnings.warn(
./xarray/core/extensions.py: warnings.warn(
./xarray/core/utils.py: warnings.warn(
./xarray/core/utils.py: warnings.warn(
./xarray/core/utils.py: warnings.warn(
./xarray/core/utils.py: warnings.warn(
./xarray/core/utils.py: warnings.warn(message, category=category, stacklevel=stacklevel)
./xarray/backends/plugins.py: warnings.warn(
./xarray/backends/plugins.py: warnings.warn(f"Engine {name!r} loading failed:\n{ex}", RuntimeWarning)
./xarray/backends/plugins.py: warnings.warn(f"{engine!r} fails while guessing", RuntimeWarning)
./xarray/backends/plugins.py: warnings.warn(f"{engine!r} fails while guessing", RuntimeWarning)
./xarray/backends/pynio_.py: warnings.warn(
./xarray/backends/file_manager.py: warnings.warn(
./xarray/backends/zarr.py: warnings.warn(
./xarray/util/deprecation_helpers.py: warnings.warn(
./xarray/plot/facetgrid.py: warnings.warn("Ignoring col_wrap since both col and row were passed")
./xarray/plot/facetgrid.py: warnings.warn(
./xarray/plot/facetgrid.py: warnings.warn(
./xarray/plot/dataarray_plot.py: warnings.warn(msg, DeprecationWarning, stacklevel=2)
./xarray/plot/dataarray_plot.py: warnings.warn(
./xarray/plot/dataarray_plot.py: warnings.warn(msg, DeprecationWarning, stacklevel=2)
./xarray/plot/dataset_plot.py: warnings.warn(msg, DeprecationWarning, stacklevel=2)
./xarray/plot/utils.py: warnings.warn(
./xarray/plot/utils.py: warnings.warn(
./xarray/coding/cftimeindex.py: warnings.warn(
./xarray/coding/variables.py: warnings.warn(
./xarray/coding/variables.py: warnings.warn(
./xarray/coding/variables.py: warnings.warn(
./xarray/coding/times.py: warnings.warn(warning_msg, SerializationWarning)
./xarray/coding/times.py: warnings.warn(
./xarray/namedarray/core.py: warnings.warn(

@hmaarrfk

Copy link
Copy Markdown
ContributorAuthor

I've been using a custom linting script with ripgrep to enforce easy rules for style like this.

@hmaarrfk

Copy link
Copy Markdown
ContributorAuthor

Thanks again for merging!

@max-sixty

Copy link
Copy Markdown
Collaborator

Thanks again for merging!

Thank you @hmaarrfk !

@max-sixty

Copy link
Copy Markdown
Collaborator

I've been using a custom linting script with ripgrep to enforce easy rules for style like this.

Yes, I've had some success with pre-commit for this!

@hmaarrfk

Copy link
Copy Markdown
ContributorAuthor

it doesn't even have to be so fancy. just

# put it in a run_lint.sh script
matches=$(rg --line-number "^from warnings import warn")
if [ $? == "0" ]; then
echo To point the user to the correct line of code
echo We prefer to use `from xarray.core.utils import emit_user_level_warning`
echo "${matches}"
exit 1
fi
matches=$(rg --line-number "warnings\.warn")
if [ $? == "0" ]; then
echo To point the user to the correct line of code
echo We prefer to use `from xarray.core.utils import emit_user_level_warning`
echo "${matches}"
exit 1
fi

@max-sixty

Copy link
Copy Markdown
Collaborator

Yeah, the nice thing about pre-commit is that it runs on every commit already, and runs locally on every platform, and isn't new to most folks. But the script totally fine too!

dcherian added a commit to dcherian/xarray that referenced this pull request Dec 18, 2023
* main: (26 commits)
Filter null values before plotting (pydata#8535)
Update concat.py (pydata#8538)
Add getitem to array protocol (pydata#8406)
Added option to specify weights in xr.corr() and xr.cov() (pydata#8527)
Filter out doctest warning (pydata#8539)
Bump actions/setup-python from 4 to 5 (pydata#8540)
Point users to where in their code they should make mods for Dataset.dims (pydata#8534)
Add Cumulative aggregation (pydata#8512)
dev whats-new
Whats-new for 2023.12.0 (pydata#8532)
explicitly skip using `__array_namespace__` for `numpy.ndarray` (pydata#8526)
Add `eval` method to Dataset (pydata#7163)
Deprecate ds.dims returning dict (pydata#8500)
test and fix empty xindexes repr (pydata#8521)
Remove PR labeler bot (pydata#8525)
Hypothesis strategy for generating Variable objects (pydata#8404)
Use numbagg for `rolling` methods (pydata#8493)
Bump pypa/gh-action-pypi-publish from 1.8.10 to 1.8.11 (pydata#8514)
fix RTD docs build (pydata#8519)
Fix type of `.assign_coords` (pydata#8495)
...
dcherian added a commit to dcherian/xarray that referenced this pull request Dec 20, 2023
* main: (58 commits)
Adapt map_blocks to use new Coordinates API (pydata#8560)
add xeofs to ecosystem.rst (pydata#8561)
Offer a fixture for unifying DataArray & Dataset tests (pydata#8533)
Generalize cumulative reduction (scan) to non-dask types (pydata#8019)
Filter null values before plotting (pydata#8535)
Update concat.py (pydata#8538)
Add getitem to array protocol (pydata#8406)
Added option to specify weights in xr.corr() and xr.cov() (pydata#8527)
Filter out doctest warning (pydata#8539)
Bump actions/setup-python from 4 to 5 (pydata#8540)
Point users to where in their code they should make mods for Dataset.dims (pydata#8534)
Add Cumulative aggregation (pydata#8512)
dev whats-new
Whats-new for 2023.12.0 (pydata#8532)
explicitly skip using `__array_namespace__` for `numpy.ndarray` (pydata#8526)
Add `eval` method to Dataset (pydata#7163)
Deprecate ds.dims returning dict (pydata#8500)
test and fix empty xindexes repr (pydata#8521)
Remove PR labeler bot (pydata#8525)
Hypothesis strategy for generating Variable objects (pydata#8404)
...
dcherian added a commit to dcherian/xarray that referenced this pull request Jan 4, 2024
commit 0a0f800
Merge: 33c803341d33f5
Author: Deepak Cherian <dcherian@users.noreply.github.com>
Date: Tue Jan 2 20:42:51 2024 -0700
Merge branch 'main' into depr-groupby-squeeze-2
commit 33c8033
Author: Deepak Cherian <deepak@cherian.net>
Date: Tue Jan 2 20:40:42 2024 -0700
Don't skip for resampling
commit d7be352
Author: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Date: Wed Jan 3 03:24:13 2024 +0000
[pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
commit d13fa0e
Author: Deepak Cherian <dcherian@users.noreply.github.com>
Date: Tue Jan 2 20:23:43 2024 -0700
Apply suggestions from code review
Co-authored-by: Michael Niklas <mick.niklas@gmail.com>
commit dd6ea53
Author: Deepak Cherian <deepak@cherian.net>
Date: Thu Dec 21 19:29:40 2023 -0700
Silence more warnings
commit 44e5a41
Author: Deepak Cherian <deepak@cherian.net>
Date: Thu Dec 21 19:21:06 2023 -0700
minimize test mods
commit 94c1c1f
Author: Deepak Cherian <deepak@cherian.net>
Date: Thu Dec 21 18:55:46 2023 -0700
Add tests for pydata#8263
commit 0ab4eb6
Author: Deepak Cherian <deepak@cherian.net>
Date: Thu Dec 21 18:47:41 2023 -0700
Fix typing
commit a064430
Merge: d6a3f2d03ec3cb
Author: Deepak Cherian <deepak@cherian.net>
Date: Thu Dec 21 18:47:04 2023 -0700
Merge branch 'main' into depr-groupby-squeeze-2
* main:
Fix mypy type ignore (pydata#8564)
Support for the new compression arguments. (pydata#7551)
FIX: reverse index output of bottleneck move_argmax/move_argmin functions (pydata#8552)
Adapt map_blocks to use new Coordinates API (pydata#8560)
add xeofs to ecosystem.rst (pydata#8561)
Offer a fixture for unifying DataArray & Dataset tests (pydata#8533)
Generalize cumulative reduction (scan) to non-dask types (pydata#8019)
commit d6a3f2d
Author: Deepak Cherian <deepak@cherian.net>
Date: Thu Dec 21 18:46:50 2023 -0700
Fix generator for aggregations
commit 97f1695
Author: Deepak Cherian <deepak@cherian.net>
Date: Tue Dec 19 10:58:11 2023 -0700
Fix docs
commit 5b33b98
Author: Deepak Cherian <deepak@cherian.net>
Date: Sun Dec 17 20:35:53 2023 -0700
fix whats-new
commit 80b2b36
Author: Deepak Cherian <deepak@cherian.net>
Date: Sun Dec 17 20:26:17 2023 -0700
Reduce more warnings
commit 5f6f4ea
Merge: a57d4ae2971994
Author: Deepak Cherian <deepak@cherian.net>
Date: Sat Dec 16 20:33:13 2023 -0700
Merge branch 'main' into depr-groupby-squeeze-2
* main: (26 commits)
Filter null values before plotting (pydata#8535)
Update concat.py (pydata#8538)
Add getitem to array protocol (pydata#8406)
Added option to specify weights in xr.corr() and xr.cov() (pydata#8527)
Filter out doctest warning (pydata#8539)
Bump actions/setup-python from 4 to 5 (pydata#8540)
Point users to where in their code they should make mods for Dataset.dims (pydata#8534)
Add Cumulative aggregation (pydata#8512)
dev whats-new
Whats-new for 2023.12.0 (pydata#8532)
explicitly skip using `__array_namespace__` for `numpy.ndarray` (pydata#8526)
Add `eval` method to Dataset (pydata#7163)
Deprecate ds.dims returning dict (pydata#8500)
test and fix empty xindexes repr (pydata#8521)
Remove PR labeler bot (pydata#8525)
Hypothesis strategy for generating Variable objects (pydata#8404)
Use numbagg for `rolling` methods (pydata#8493)
Bump pypa/gh-action-pypi-publish from 1.8.10 to 1.8.11 (pydata#8514)
fix RTD docs build (pydata#8519)
Fix type of `.assign_coords` (pydata#8495)
...
commit a57d4ae
Author: Deepak Cherian <deepak@cherian.net>
Date: Fri Dec 1 21:36:04 2023 -0700
Test one more warning
commit bf8139d
Author: Deepak Cherian <dcherian@users.noreply.github.com>
Date: Fri Dec 1 21:33:45 2023 -0700
Update xarray/tests/test_groupby.py
commit 4e9a063
Author: Deepak Cherian <deepak@cherian.net>
Date: Fri Dec 1 21:10:14 2023 -0700
Set squeeze=None for Dataset too
commit c2e576e
Author: Deepak Cherian <deepak@cherian.net>
Date: Fri Dec 1 20:54:17 2023 -0700
Fix first, last
commit 6d8e822
Author: Deepak Cherian <deepak@cherian.net>
Date: Fri Dec 1 20:46:21 2023 -0700
better warning
commit 62c334b
Author: Deepak Cherian <deepak@cherian.net>
Date: Fri Dec 1 20:45:17 2023 -0700
silence warnings
commit b7805a8
Author: dcherian <deepak@cherian.net>
Date: Tue Aug 15 10:54:25 2023 -0600
Deprecate `squeeze` in GroupBy.
Closespydata#2157
Sign up for freeto 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.

3 participants

@hmaarrfk@max-sixty@dcherian