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

Rotate teapot for flip_z() doctest example #2885

Merged
merged 4 commits into from
Jun 28, 2022

Conversation

adeak
Copy link
Member

@adeak adeak commented Jun 27, 2022

The flip_z() doctest result looks like this:

two identical-looking teapots, before and after flip_z()

The teapots look identical because the xy plane in this configuration is parallel to the symmetry plane of the teapot.

If we first rotate the teapot around x we get a more meaningful result:

two teapots, one of them upside down

This makes the test slightly different from flip_x() and flip_y(), but it's probably more important to have the example be useful in isolation.

@adeak adeak added the documentation Anything related to the documentation/website label Jun 27, 2022
@codecov
Copy link

codecov bot commented Jun 27, 2022

Codecov Report

Merging #2885 (f8d03a7) into main (6bdedb7) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2885   +/-   ##
=======================================
  Coverage   94.04%   94.04%           
=======================================
  Files          76       76           
  Lines       16428    16428           
=======================================
  Hits        15449    15449           
  Misses        979      979           

akaszynski
akaszynski previously approved these changes Jun 27, 2022
Copy link
Member

@akaszynski akaszynski left a comment

Choose a reason for hiding this comment

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

LGTM. I thought the labeler was fixed...

@akaszynski
Copy link
Member

LGTM. I thought the labeler was fixed...

Actually, seems like this fails for forks... @tkoyama010, let's find a way to disable this for forks.

@adeak
Copy link
Member Author

adeak commented Jun 27, 2022

At worst I can start using feature branches on the main repo. Vast majority of PRs seems to come from members anyway.

@akaszynski
Copy link
Member

At worst I can start using feature branches on the main repo. Vast majority of PRs seems to come from members anyway.

I wouldn't want to discourage forks. Let's get it working for forks.

@tkoyama010
Copy link
Member

@akaszynski Could you please check that the setting is the follwing?
f0a5477217b16cc2c04144ee1adb6614.png

@tkoyama010 tkoyama010 removed the documentation Anything related to the documentation/website label Jun 27, 2022
@akaszynski
Copy link
Member

@akaszynski Could you please check that the setting is the follwing?

@banesullivan, are we fine with upping the scope on GITHUB_TOKEN? I doubt that we'll run into issues, but it's an additional security risk.

@tkoyama010 tkoyama010 added the documentation Anything related to the documentation/website label Jun 27, 2022
@tkoyama010
Copy link
Member

I reverted the trigger of labeling which is dropping in #2877.

@banesullivan
Copy link
Member

@banesullivan, are we fine with upping the scope on GITHUB_TOKEN? I doubt that we'll run into issues, but it's an additional security risk.

I'm comfortable with this, just so long as we aren't using a Personal Access Token (PAT) which can have elevated permissions

akaszynski
akaszynski previously approved these changes Jun 27, 2022
Copy link
Member

@akaszynski akaszynski left a comment

Choose a reason for hiding this comment

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

Still LGTM.

@akaszynski
Copy link
Member

@akaszynski Could you please check that the setting is the follwing? f0a5477217b16cc2c04144ee1adb6614.png

It indeed is set.

Copy link
Member

@tkoyama010 tkoyama010 left a comment

Choose a reason for hiding this comment

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

LGTM

@tkoyama010 tkoyama010 merged commit 9f78425 into pyvista:main Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Anything related to the documentation/website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants