Skip to content

Resolves #26421 Added an example for fig comparison decorator #26538

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 28 commits into from
Aug 28, 2023
Merged

Resolves #26421 Added an example for fig comparison decorator #26538

merged 28 commits into from
Aug 28, 2023

Conversation

SunSummoner
Copy link
Contributor

@SunSummoner SunSummoner commented Aug 16, 2023 •
edited
Loading

PR summary

Added an example of figure comparison decorator check_figures_equal() in testing doc. @story645 please review and let me know if I made some mistakes.

PR checklist

github-actions[bot]
github-actions bot reviewed Aug 16, 2023
View reviewed changes
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
ksunden reviewed Aug 16, 2023
View reviewed changes
@SunSummoner @ksunden
Update doc/devel/testing.rst
0ff2125 
Fixed trailing spaces

Co-authored-by: Kyle Sunden <git@ksunden.space>
@SunSummoner
Copy link
Contributor Author

@ksunden thank you!

story645
story645 requested changes Aug 16, 2023
View reviewed changes
Copy link
Member

@story645 story645 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 tackling this! I think it's a good start but the example needs to show the use of both fig_ref and fig_test

doc/devel/testing.rst Outdated
Comment on lines 136 to 137
then collect the drawn results and compare them. For example, this test draws
the same circle on the figures with two different methods::
Copy link
Member

Choose a reason for hiding this comment

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

maybe me more explicit about the two methods (drawing via adding circle patch vs drawing via plotting a circle)? And change the name of the test method to whichever method you're testing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate the second part? I am not quite sure what you mean. Do you mean change the name of the test method in the code?

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean change the name of the test method in the code?

Exactly! Something like test_parametric_circle_plot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay! I'll edit it.

@SunSummoner
Update testing.rst
a182f3f 
Made changes to add fig_test and made the documentation more cohesive.
@SunSummoner
Update testing.rst
f1726fc 
Removed import pytest and added the trailing space.
@SunSummoner
Update testing.rst
4edea20
story645
story645 approved these changes Aug 17, 2023
View reviewed changes
Copy link
Member

@story645 story645 left a comment

Choose a reason for hiding this comment

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

Slight wording change and preference for more explicit test name, but this can still be merged if you don't have time to make those changes-I'll just give it a day or so before merging to give you time to respond.

@ksunden
Copy link
Member

ksunden commented Aug 17, 2023

Looks like there remains some trailing whitespace, probably in the paragraphs (Github highlights it when it was on its own line, so that is all I saw earlier, but CI indicates it is still there)

@SunSummoner @story645
Update doc/devel/testing.rst
302aa4e 
Co-authored-by: hannah <story645@gmail.com>
@SunSummoner
Copy link
Contributor Author

Slight wording change and preference for more explicit test name, but this can still be merged if you don't have time to make those changes-I'll just give it a day or so before merging to give you time to respond.

I'll try to edit it but I am good with you merging it. :)

@SunSummoner
Copy link
Contributor Author

SunSummoner commented Aug 17, 2023 •
edited
Loading

Looks like there remains some trailing whitespace, probably in the paragraphs (Github highlights it when it was on its own line, so that is all I saw earlier, but CI indicates it is still there)

How can I remove all of those?

@SunSummoner
Update testing.rst
bb3a00e 
Tried to remove most of the trailing spaces and changed the name of the test.
@story645
Copy link
Member

How can I remove all of those?

Have you installed the pre-commit hooks? They should do that automatically.

https://matplotlib.org/devdocs/devel/development_setup.html#install-pre-commit-hooks-optional

@SunSummoner
Copy link
Contributor Author

How can I remove all of those?

Have you installed the pre-commit hooks? They should do that automatically.

https://matplotlib.org/devdocs/devel/development_setup.html#install-pre-commit-hooks-optional

I did not have them but I have installed them now. Thank you! Do I have to make new commit to remove the trailing spaces?

@story645
Copy link
Member

Do I have to make new commit to remove the trailing spaces

Kinda yes - you add and commit the file after the pre-commit hook runs & only push up once clean. Also can you rebase the file or would you like help or for us to do it on merge?

@SunSummoner
Copy link
Contributor Author

Do I have to make new commit to remove the trailing spaces

Kinda yes - you add and commit the file after the pre-commit hook runs & only push up once clean. Also can you rebase the file or would you like help or for us to do it on merge?

I tried to commit to the file but it failed the don't commit to main branch test. I have never used pre-commit before but I tried to push the changes but I was not able to. It would be great if you could help me with or instruct how to do it. Thank you.

@story645
Copy link
Member

story645 commented Aug 18, 2023 •
edited
Loading

I tried to commit to the file but it failed the don't commit to main branch test

That's not a big deal because this is a relatively small document change, but in the future please make a feature branch - and I really do hope you do more PRs cause I think you wrote really well here.

So to push ignoring a pre-commit, you can add in a --no-verify flag to the commit where the only failing test is the no-commit-to-main:

git commit doc/devel/testing.rst -m 'Resolving pre-commit-hook changes' --no-verify

Then to do a rebase, which editor are you using?

@SunSummoner
Copy link
Contributor Author

I tried to commit to the file but it failed the don't commit to main branch test

That's not a big deal because this is a relatively small document change, but in the future please make a feature branch - and I really do hope you do more PRs cause I think you wrote really well here.

So to push ignoring a pre-commit, you can add in a --no-verify flag to the commit where the only failing test is the no-commit-to-main:

git commit doc/devel/testing.rst -m 'Resolving pre-commit-hook changes' --no-verify

Then to do a rebase, which editor are you using?

I'll keep in mind to create a feature branch next time. Thank you and I'd love to contribute and do more PRs. I got to learn a lot from this one.

Thank you. I ran the command worked and its working well. I use usually use VSCode as an editor.

@story645 story645 added this to the v3.7-doc milestone Aug 18, 2023
@story645
Copy link
Member

story645 commented Aug 18, 2023 •
edited
Loading

Here's a really good tutorial for git rebase in vscode https://www.youtube.com/watch?v=3o_01F04bZ4
(ETA: rebase is to upstream/main)

and if it seems too much right now just let me know cause looks like I can squash merge this

@SunSummoner
Copy link
Contributor Author

Here's a really good tutorial for git rebase in vscode https://www.youtube.com/watch?v=3o_01F04bZ4 (ETA: rebase is to upstream/main)

and if it seems too much right now just let me know cause looks like I can squash merge this

I saw the video and it really helped but I am afraid I'll need some practice in rebasing, so it would be great if you squash merge this. Thank you!

@story645 story645 self-requested a review August 22, 2023 06:20
story645
story645 requested changes Aug 22, 2023
View reviewed changes
Copy link
Member

@story645 story645 left a comment

Choose a reason for hiding this comment

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

The absolute must fix is the crossrefs because that's why the doc build is failing.

SunSummoner and others added 3 commits August 22, 2023 13:40
@SunSummoner @story645
Update testing.rst
461494e 
Co-authored-by: hannah <story645@gmail.com>
@SunSummoner @story645
Update testing.rst
4add791 
Co-authored-by: hannah <story645@gmail.com>
@SunSummoner @story645
Update testing.rst
02203f1 
Co-authored-by: hannah <story645@gmail.com>
story645
story645 requested changes Aug 22, 2023
View reviewed changes
story645
story645 reviewed Aug 22, 2023
View reviewed changes
SunSummoner and others added 3 commits August 22, 2023 23:06
@SunSummoner @story645
Update testing.rst
4d7c58f 
Co-authored-by: hannah <story645@gmail.com>
@SunSummoner @story645
Update testing.rst
323fd7b 
Co-authored-by: hannah <story645@gmail.com>
@SunSummoner @story645
Update testing.rst
8e51433 
Co-authored-by: hannah <story645@gmail.com>
story645
story645 requested changes Aug 23, 2023
View reviewed changes
Copy link
Member

@story645 story645 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Sorry didn't make it clear that patches import should be w/ the others.

@SunSummoner @story645
Update doc/devel/testing.rst
30251e3 
Co-authored-by: hannah <story645@gmail.com>
@SunSummoner
Copy link
Contributor Author

Sorry didn't make it clear that patches import should be w/ the others.

Should I add an import statement for pytest too? Or is it given since it was written in doc.

@SunSummoner @story645
Update doc/devel/testing.rst
3d74be9 
Co-authored-by: hannah <story645@gmail.com>
@story645
Copy link
Member

Should I add an import statement for pytest too? Or is it given since it was written in doc.

it's given since it was written in doc.

@story645 story645 self-assigned this Aug 24, 2023
story645
story645 reviewed Aug 28, 2023
View reviewed changes
@story645 story645 merged commit a84c911 into matplotlib:main Aug 28, 2023
@meeseeksmachine meeseeksmachine mentioned this pull request Aug 28, 2023
Merged
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Aug 28, 2023
@SunSummoner @meeseeksmachine
Backport PR matplotlib#26538: Resolves matplotlib#26421 Added an exam…
84421cc 
…ple for fig comparison decorator
@lumberbot-app
Copy link

lumberbot-app bot commented Aug 28, 2023

Something went wrong ... Please have a look at my logs.

It seems that the branch you are trying to backport to does not exist.

@story645
Copy link
Member

Thank you so much for seeing this PR through, congrats on your first merged PR, and we hope to see you again!

@SunSummoner
Copy link
Contributor Author

Thank you so much for seeing this PR through, congrats on your first merged PR, and we hope to see you again!

Thank you for helping me through it. I got to learn a lot and I look forward to contributing again!!

story645 added a commit that referenced this pull request Aug 28, 2023
@story645
Merge pull request #26610 from meeseeksmachine/auto-backport-of-pr-26…
923d8fb 
…538-on-v3.7.x

Backport PR #26538 on branch v3.7.x (Resolves #26421 Added an example for fig comparison decorator)
@QuLogic QuLogic modified the milestones: v3.7-doc, v3.7.3 Sep 1, 2023
@QuLogic QuLogic mentioned this pull request Sep 13, 2023
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic left review comments

@ksunden ksunden ksunden left review comments

@github-actions github-actions[bot] github-actions[bot] left review comments

@story645 story645 story645 approved these changes

Assignees

@story645 story645

Labels
None yet
Projects
First Time Contributors
Status: Merged
Milestone
v3.7.3
Development

Successfully merging this pull request may close these issues.

[Doc]: demo testing comparison decorator
4 participants
@SunSummoner @ksunden @story645 @QuLogic