Skip to content

Conversation

@johnomotani
Copy link
Contributor

@johnomotani johnomotani commented Mar 31, 2021

Use broadcast_like if either x or y inputs are 2d to ensure that both have dimensions in the same order as the DataArray being plotted. Convert to numpy arrays after possibly using broadcast_like. Simplifies code, and fixes #5097 (bug when dimensions have the same size).

@dcherian

IIRC the "resolving intervals" section later will break and is somewhat annoying to fix. This is why the current ugly code exists.

This change seems to 'just work', and unit tests pass. Is there some extra check that needs doing to make sure "resolving intervals" is behaving correctly?

I can't think of a unit test that would have caught #5097, since even when the bug happens, a plot is produced without errors or warnings. If anyone has an idea, suggestions/pushes welcome!

Use broadcast_like if either `x` or `y` inputs are 2d to ensure that
both have dimensions in the same order as the DataArray being plotted.
Convert to numpy arrays after possibly using broadcast_like. Simplifies
code, and fixes pydata#5097 (bug when dimensions have the same size).
@dcherian
Copy link
Contributor

This change seems to 'just work', and unit tests pass. Is there some extra check that needs doing to make sure "resolving intervals" is behaving correctly?

OK I must be misremembering then.

@johnomotani johnomotani mentioned this pull request Mar 31, 2021
5 tasks
Copy link
Collaborator

@mathause mathause left a comment

Choose a reason for hiding this comment

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

Thanks for your patience! In your MVCE (#5097) compare h1.get_paths()[0] and h2.get_paths()[0]. Thus, you can use h2.get_paths() for a test e.g.:

v = h2.get_paths()[0].vertices
assert not np.all(v[:, 0] == v[:, 1])

(in the MVCE all x and y coords are equal i.e. it's a diagonal with width 0).

I suggest merging this (pending a test if you are up to it), and if something breaks the interval resolution that is not tested we fix it later...

@johnomotani
Copy link
Contributor Author

Thanks @mathause! I've used your suggestion to turn my MVCE into a test which fails on master, but passes with the fix in this PR.

Copy link
Collaborator

@mathause mathause left a comment

Choose a reason for hiding this comment

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

LGTM. Let's see if someone else wants to comment and I'll merge in a day or two.

@mathause
Copy link
Collaborator

Thanks @johnomotani - fingers crossed that this does not break anything.

@mathause mathause merged commit b2351cb into pydata:master Apr 22, 2021
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.

2d plots may fail for some choices of x and y

3 participants