Fix map_overlap trimming behavior when drop_axis is not None#7894
Fix map_overlap trimming behavior when drop_axis is not None#7894
Conversation
…form have to remove any dropped axes from depth and boundary when calling trim_internal
|
@jakirkham could you (or someone else) please start the CI workflows? Thanks! |
dask/array/tests/test_overlap.py
Outdated
|
|
||
|
|
||
| def test_map_overlap_trim_using_drop_axis_and_different_depths(): | ||
| x = da.ones((5, 10), dtype=float) |
There was a problem hiding this comment.
I'd like to see some non-2D examples in the tests, as well
There was a problem hiding this comment.
okay. I should also add a non-uniform boundary
dask/array/overlap.py
Outdated
| # note that keys are relabeled to match values in range(x.ndim) | ||
| x_depth = {n: x_depth[ax] for n, ax in enumerate(x_axis)} | ||
| x_boundary = {n: x_boundary[ax] for n, ax in enumerate(x_axis)} | ||
| return trim_internal(x, x_depth, x_boundary) |
There was a problem hiding this comment.
I feel like the x_ prefix on depth, axis and boundary is more confusing than not.
Yep done :) |
vary both depth and boundary across axes test many drop_axis combinations test values via assert_array_almost_equal
|
I have updated the test cases here. One other issue I noticed when developing the test cases is that the use of negative values such as |
Huh, that's weird. On my end I still see " 4 workflows awaiting approval. First-time contributors need a maintainer to approve running workflows." |
I think that is because I pushed new commits afterwards. Annoyingly, you have to approve every time |
This seems like an important thing to fix, but probably makes better sense to put in a new PR. (Bundling stuff together makes it kind of hard to look back through the commit log at the PR titles and figure out when something was introduced) |
|
I think the issue with negative values in drop_axis is not exclusive to the newly added code. The following line in the underlying Line 686 in 714ff4b |
Sadly each new commit needs to be approved |
|
I fixed a bug in the test case. can someone approve the workflows again here? |
|
It seems like this is good to go. Is that right @GenevieveBuckley? |
|
This looks great! Thanks so much for the contribution @grlee77!! |
|
Can one of the admins verify this patch? |
5 participants
This is a bug fix PR
It regenerates the depth and boundary variables, taking
drop_axisinto account before callingtrim_internalblack dask/flake8 dask/isort dask