Skip to content

Added test for tricontour #27545

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

Closed
wants to merge 1 commit into from
Closed

Conversation

szhukovska
Copy link

PR summary

Added 3 tests for tricontour (issue #26864): datetime on y only, on x only and on x and y axes

PR checklist

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

@ksunden
Copy link
Member

ksunden commented Dec 19, 2023

Thank you for your interest in improving Matplotlib!

However, this portion of that issue has already received a PR (#27494).
In general, barring significant technical advantages, we tend to prefer to give the first author to open a PR the opportunity to finish their contribution.

And in looking into it we discovered that the tri* plots are not actually properly handling unit-ful inputs, so we raised #27499. Your plot is hiding these failures because it a) calls plot so the axis units get set, when they are not properly set by tricontour alone (but should be) and b) happen to "work" because they use datetime64[D] dtype which coincides with matplotlib's floating point representation of dates for axes, but it does not actually pass through the units code so if you used a different scale (e.g. datetime64[s]) then you would get the wrong thing plotted. (or indeed changed the epoch date for mpl from 1970-01-01)

@szhukovska szhukovska closed this Dec 19, 2023
@szhukovska
Copy link
Author

Alright, thanks for the quick feedback!

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.

2 participants