Deprecation warning for default value of boundary kwarg in map_overlap by GenevieveBuckley · Pull Request #8397 · dask/dask

GenevieveBuckley · 2021-11-18T10:09:20Z

This PR is the first step in updating the default boundary kwarg value in Dask's map_overlap function. The current default value is "reflect", in future this will be changed to "none".

We want to do this primarily for performance reasons, as discussed here: #8391

Questions:

Should we choose a specific version to change this in? Something 6 months or a year away?
Do we need to do this as a two step process? Potentially I could use the disutils LooseVersion to check whether the dask version is above or below a certain number, so this change would turn on automatically.
- ...although then I guess you'd still need a second step to prune out that stuff at a later date, it's just less important to remember to do it at a specific time
Is FutureWarning correct to use here? This seems the most commonly used in the Dask codebase, although there is also a PendingDeprecationWarning used a handful of times, so I thought I should check.

Related to Change default boundary value to "none" for dask map_overlap #8391
~~Tests added / passed~~
Passes pre-commit run --all-files

… map_overlap

GenevieveBuckley · 2021-11-18T10:10:23Z

cc @jakirkham

dask/array/overlap.py

GenevieveBuckley · 2021-11-19T06:27:21Z

There are a number of tests that are failing because of the FutureWarning added here. I've updated the tests to explicitly specify what boundary value they need here.

jakirkham

Thanks Genevieve! This looks good 😄

Had a couple comments on the warning message. Otherwise LGTM

jakirkham · 2021-11-19T06:30:16Z

dask/array/overlap.py

+        # Default boundary value is set in the function named "coerce_boundary"
+        warnings.warn(
+            "Default 'boundary' argument value will change from 'reflect' "
+            "to 'none' in future versions. "


Should we specify a version here?

I think it can be relatively soon. Maybe February?

I'd like to specify a version.

February seems too early though, lots of people will be on holiday in December and January, so it's quite likely people won't see this at all before it changes. (Although I'm sure plenty of scientists have code they run infrequently, like once every six months or so on a new dataset, so maybe it's impractical to wait a huge amount of time.)

Mind you, the only visible differences should be at the very outer edges of a Dask array, so the impact of the change is pretty small. Maybe it's ok to change it soon and have people's first introduction be a pleasant surprise things are running faster.

We can always change the version if we don't feel ready. But I tend to agree that I don't think this will negatively impact many people. From the responses on this, the FutureWarning won't really hit them either since they set this kwarg. So I'm fine with whatever you choose.

Ok, I'll say March then, 2021.03.0
It's an arbitrary choice, but we should pick something.

dask/array/overlap.py

Co-authored-by: jakirkham <jakirkham@gmail.com>

dask/array/core.py

GenevieveBuckley · 2021-11-23T02:30:32Z

Ok, latest commits should fix up those doctest errors (doctests were not explicitly specifying boundary= kwarg and then failing when the warning was raised)

GenevieveBuckley · 2021-11-25T07:30:34Z

...I don't really understand why the macOS tests are failing here. Something's a bit weird. I'm going to investigate on my personal laptop & see if I can reproduce it.

Locally on a Mac, nothing fails when I run pytest dask/tests/test_layers.py (which is where the CI test failure is happening. But when I try running the whole test suite, I get a pytest error "Too many open files". I thought that might be this problem again, but it doesn't look like it. My ulimit is reported as unlimited, and trying the trick with ulimit -n 2048 doesn't change anything.

I've tried with both python 3.8 and python 3.9 environments, and with the main and boundary-default-warning branches. They don't seem to affect things much.

I see the CI test suite passing for other recent PRs, so I don't think anything is broken on the main branch. I might try merging main into this branch, to see if that helps. I don't see why it would, but worth a shot.

GenevieveBuckley · 2021-11-26T03:37:16Z

I should add - the kinds of test errors I'm seeing on my laptop are not the same event loop errors that are happening in the CI.

ERROR dask/array/tests/test_xarray.py - RuntimeError: There is no current eve...
ERROR dask/bytes/tests/test_hdfs.py - RuntimeError: There is no current event...
ERROR dask/tests/test_distributed.py - RuntimeError: There is no current even...
ERROR dask/tests/test_layers.py - RuntimeError: There is no current event loo...
ERROR gw0

Here's hoping those are just a bit of a fluke.

jsignell

This looks great! Thanks for being so thorough!

GenevieveBuckley · 2021-11-29T23:27:23Z

I've just scheduled an email that should hopefully remind me to make a follow up PR to this in the last week of February (when we should actually change the kwarg default value). Fingers crossed the reminder works!

jsignell · 2021-11-30T15:59:19Z

Whoa. I've never thought of doing something like that.

…8743) Followup to PR #8397 We've had a FutureWarning up for a few months about an upcoming change to the default 'boundary' kwarg value in `map_overlap`, so now is the time to change it. Previous default was `"reflect"`, new default will be "None". The reason for this change is that it makes the code run a lot faster, and for most people the overlap depth is sufficient and they should not require additional boundary handling. See #8391 for a full discussion.

GenevieveBuckley added 3 commits November 18, 2021 20:52

Deprecation warning for default value of boundary kwarg in map_overlap

98972e1

Fix indentation level

e98d0a5

Add comment signposting location to change default boundary value for…

75fa9de

… map_overlap

github-actions bot added the array label Nov 18, 2021

jsignell reviewed Nov 18, 2021

View reviewed changes

dask/array/overlap.py Outdated Show resolved Hide resolved

GenevieveBuckley added 2 commits November 19, 2021 16:24

Expand FutureWarning for boundary kwarg in map_overlap

b1d9a34

Explicitly specify boundary kwarg in map_overlap tests

f1c921f

Update test_sparse with explicit boundary kwarg in map_overlap

f66aad1

jakirkham reviewed Nov 19, 2021

View reviewed changes

GenevieveBuckley and others added 2 commits November 22, 2021 19:44

Update dask/array/overlap.py

1d4291a

Co-authored-by: jakirkham <jakirkham@gmail.com>

Update doctests for map_overlap

26a1785

GenevieveBuckley commented Nov 23, 2021

View reviewed changes

dask/array/core.py Show resolved Hide resolved

GenevieveBuckley added 2 commits November 23, 2021 13:13

Specify map_overlap kwarg change from version 2022.03.0 onwards

a181157

Update doctests in overlap.py map_overlap to avoid warning

03ff479

Specify map_overlap boundary kwarg for test_layers.py

e0193e7

Merge branch 'main' into boundary-default-warning

2db7ac8

jsignell approved these changes Nov 29, 2021

View reviewed changes

jsignell merged commit 848388f into dask:main Nov 29, 2021

GenevieveBuckley deleted the boundary-default-warning branch November 29, 2021 23:27

GenevieveBuckley mentioned this pull request Feb 21, 2022

Followup to PR 8397 map_overlap default boundary kwarg #8743

Merged

3 tasks

mkcor mentioned this pull request Feb 21, 2026

Remove mention of obsolete default value for 'boundary' parameter. #12304

Merged

Uh oh!

Conversation

GenevieveBuckley commented Nov 18, 2021

Uh oh!

GenevieveBuckley commented Nov 18, 2021

Uh oh!

Uh oh!

GenevieveBuckley commented Nov 19, 2021

Uh oh!

jakirkham left a comment

Choose a reason for hiding this comment

Uh oh!

jakirkham Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

jsignell Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

GenevieveBuckley Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

jsignell Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

GenevieveBuckley Nov 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

GenevieveBuckley commented Nov 23, 2021

Uh oh!

GenevieveBuckley commented Nov 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GenevieveBuckley commented Nov 26, 2021

Uh oh!

jsignell left a comment

Choose a reason for hiding this comment

Uh oh!

GenevieveBuckley commented Nov 29, 2021

Uh oh!

jsignell commented Nov 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

GenevieveBuckley Nov 23, 2021 •

edited

Loading

GenevieveBuckley commented Nov 25, 2021 •

edited

Loading