Skip to content

Remove datetime test for axes.pie #27117

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

Merged
merged 4 commits into from
Oct 17, 2023

Conversation

Sheepfan0828
Copy link
Contributor

@Sheepfan0828 Sheepfan0828 commented Oct 17, 2023

PR summary

The task i am working with is Axe.pie problem from the issue #26864 . The Axe.pie should be removed from the file test_datetime.py file because we cannot directly pass the datetime object into pie as a parameter to perform any of the plots. The error message below shows the case occured when datetime object is directly passed into the pie function:
屏幕截图 2023-10-17 164915

PR checklist

@Sheepfan0828
Copy link
Contributor Author

Both u7277320 and sheepfan0828 are my personal Github account

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.

@Sheepfan0828
Copy link
Contributor Author

Is it okey that some checks are not passed?

@ksunden
Copy link
Member

ksunden commented Oct 17, 2023

The linting would need to be addressed.

However, more fundamentally, this is not exercising the datetime unit machinery that is expected here, and in fact pie in general does not use the units system (and it's unclear what it even would mean).

As such I think pie should just be removed from this file all together (much like we did to table/spectral methods in #27072)

The list from the original issue/stubs was intentionally a bit broad but part of the idea was to evaluate whether it was using units at all. In this case it is not, and so it should just be removed.

@Sheepfan0828
Copy link
Contributor Author

Sheepfan0828 commented Oct 17, 2023

@ksunden Sorry, what do you mean by that? I cannot understand, can you please explain it more specific?

@ksunden
Copy link
Member

ksunden commented Oct 17, 2023

This method falls into the category that passing datetimes to it directly does not make sense and therefore should just be removed (From #26864):

iii: If passing datetime / united data to the method does not make any sense or it should not work open a PR removing that test method and similarly reference this issue.

By calling to_ordinal here, you are not passing the datetime objects, and so are not doing what this test file is trying to exercise. This is because the idea of this test file does not make sense for pie, and therefore the xfail stub (the entire method) should just be removed altogether.

@Sheepfan0828
Copy link
Contributor Author

This method falls into the category that passing datetimes to it directly does not make sense and therefore should just be removed (From #26864):

iii: If passing datetime / united data to the method does not make any sense or it should not work open a PR removing that test method and similarly reference this issue.

By calling to_ordinal here, you are not passing the datetime objects, and so are not doing what this test file is trying to exercise. This is because the idea of this test file does not make sense for pie, and therefore the xfail stub (the entire method) should just be removed altogether.

So do you mean that is the method that i am testing with (pie this case) is not accepting datetime object so that the testing function for pie should be removed from the file? If so, i need to remove it from test_datetime.py right now and then create another pull request that i have removed the pie. Is that right?

@ksunden
Copy link
Member

ksunden commented Oct 17, 2023

You can update this PR, no need to make a new one, but yes, I am saying that pie should just be removed from this file.

Copy link
Member

@ksunden ksunden left a comment

Choose a reason for hiding this comment

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

I would suggest squash merging this (unless @Sheepfan0828 you wish to squash commits see here)

@rcomer rcomer changed the title Smoke testing for axe.pie Remove datetime test for axes.pie Oct 17, 2023
@rcomer rcomer merged commit eca4a34 into matplotlib:main Oct 17, 2023
@rcomer
Copy link
Member

rcomer commented Oct 17, 2023

I took the liberty of updating the PR title as we are no longer smoke testing this method.

Thanks @Sheepfan0828 and congratulations on your first Matplotlib PR! We hope to hear from you again.

@rcomer rcomer added this to the v3.9.0 milestone Oct 17, 2023
@rcomer rcomer mentioned this pull request Oct 17, 2023
64 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants