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

Fix for all NANs in contour #25334

Merged
merged 4 commits into from Feb 27, 2023
Merged

Fix for all NANs in contour #25334

merged 4 commits into from Feb 27, 2023

Conversation

xtanion
Copy link
Contributor

@xtanion xtanion commented Feb 26, 2023

PR Summary

As discussed in #25330, we're removing the typecase to float with z.min().astype(float).
Closes #14124.

@xtanion xtanion changed the title Fix for alll NANs in contour Fix for all NANs in contour Feb 26, 2023
@oscargus
Copy link
Contributor

How does the original issue behave with this change?

(Maybe you can add the smoke test back?)

@xtanion
Copy link
Contributor Author

xtanion commented Feb 26, 2023

How does the original issue behave with this change?

I'm getting the following output with all NAN :
Screenshot from 2023-02-27 00-23-19

Yes, I'll add the tests. (we're not getting any masking errors this time)

@greglucas
Copy link
Contributor

Do you also want to change the float() cast in the log branch right below this for consistency? Once you add a test to make sure we don't warn for the all-nan case in the future this looks good to me 👍

@xtanion
Copy link
Contributor Author

xtanion commented Feb 27, 2023

done @greglucas, PTAL
Thanks

Copy link
Contributor

@greglucas greglucas left a comment

Choose a reason for hiding this comment

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

Looks good to me!

One minor nit that you can take or leave.


def test_all_nan():
x = np.array([[np.nan, np.nan], [np.nan, np.nan]])
assert_array_almost_equal(plt.contour(x).levels.tolist(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure numpy's testing will compare the list and levels array for you, so I don't think you need to convert it to a list here unless you were doing the bare assert like above.

Suggested change
assert_array_almost_equal(plt.contour(x).levels.tolist(),
assert_array_almost_equal(plt.contour(x).levels,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah right, removed to_list conversion. Thanks.

@oscargus
Copy link
Contributor

Thanks @xtanion and congratulations on your first merged PR in Matplotlib! Hope to see you around!

@oscargus oscargus merged commit 66ba515 into matplotlib:main Feb 27, 2023
@oscargus oscargus added this to the v3.8.0 milestone Feb 27, 2023
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.

plt.contour with all NaNs fails assertion in _contour.cpp
3 participants