Followup to PR 8397 map_overlap default boundary kwarg#8743
Followup to PR 8397 map_overlap default boundary kwarg#8743
Conversation
|
Note: a lot of the overlap tests still assume |
It seems fine to me to just go through and explicitly add |
|
Just let me know if you want me to finish up this PR @GenevieveBuckley :) |
I thought we already did this in the first PR. But there are still some failing tests, so I guess that didn't catch everything.
Yes please, that would be very helpful. I can point you to the spot that's causing a problem, but I can't sink much more time into this today. |
| default = "reflect" | ||
| default = "none" | ||
| if boundary is None: | ||
| boundary = default |
There was a problem hiding this comment.
It's this if condition that is leading to assertion errors in the trim tests.
If we do a presumably nonsensical thing, and set boucndary here equal to any of "reflect", "constant", "nearest", or None then all the tests pass. But when we set it equal to the string value "none" (instead of the python None) then the assertions below fail. I'm not exactly sure what is going on here.
=========================== short test summary info ============================
FAILED dask/array/tests/test_overlap.py::test_trim_internal - assert ((9, 8, ...
FAILED dask/array/tests/test_overlap.py::test_nearest_overlap - AssertionError:
FAILED dask/array/tests/test_overlap.py::test_some_0_depth - AssertionError:
FAILED dask/array/tests/test_overlap.py::test_depth_equals_boundary_length - ...
FAILED dask/array/tests/test_overlap.py::test_depth_greater_than_boundary_length
======
There was a problem hiding this comment.
Yeah I think we just need to add the boundary kwarg to all the trim_internal calls in the tests. I just pushed a commit that does that
|
Additionally, it would be good to confirm this default choice results in fewer tasks, just with a quick check of |
|
Planning to merge this once it's green since the next release will be |
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.
added/ passedpre-commit run --all-files