Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use broadcast_like for 2d plot coordinates #5099

Merged
merged 5 commits into from
Apr 22, 2021

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...

doc/whats-new.rst Outdated Show resolved Hide resolved
@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.

doc/whats-new.rst Outdated Show resolved Hide resolved
@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