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

Document that default capping changed for extrude* in 0.32 #2339

Merged
merged 1 commit into from
Mar 19, 2022

Conversation

adeak
Copy link
Member

@adeak adeak commented Mar 12, 2022

#1486 exposed the capping parameter of extrude() and extrude_rotate(). I just noticed that the False default for these kwargs is a change compared to the original VTK default of 1 (i.e. True). This PR just adds a versionchanged tag making this clear.

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

codecov bot commented Mar 12, 2022

Codecov Report

Merging #2339 (339a998) into main (dbffcb7) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2339   +/-   ##
=======================================
  Coverage   93.51%   93.51%           
=======================================
  Files          74       74           
  Lines       15709    15709           
=======================================
  Hits        14690    14690           
  Misses       1019     1019           

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.
I have verified that the #1486 change is included in v0.32.0. vtk default values are verified by the following script.

import vtk

alg = vtk.vtkLinearExtrusionFilter()
print("alg.GetCapping():", alg.GetCapping())

@adeak
Copy link
Member Author

adeak commented Mar 12, 2022

Thanks @tkoyama010 🎉

@banesullivan
Copy link
Member

I'm actually wondering if we should revert this? IMO, we should try to stick with the defaults VTK has chosen, unless there is a compelling reason not to (which is the case in many places in PyVista) - I want to make sure this change was intentional

@adeak
Copy link
Member Author

adeak commented Mar 13, 2022

I would guess that the difference is unintentional, only @akaszynski can tell for sure. But I would argue that as of 0.32 (September 2021, so for 6 months) this is our public API, so reverting it is not trivial either.

Copy link
Member

@banesullivan banesullivan left a comment

Choose a reason for hiding this comment

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

Approving this change as this has been in the public API for a while and this note is needed. I would like to defer to @adeak and @akaszynski on whether we should keep this parameter as is or revert it in a follow up PR to make sure the default value of this parameter was intentionally chosen. Perhaps there's a cost of having this default to False that is worse than the cost of changing our API? IMO, the cost of diverging from VTK's defaults is high in the long term

@akaszynski
Copy link
Member

akaszynski commented Mar 18, 2022

I think that agreement, whenever possible, with VTK is ideal. Let's change the default in this a follow-up PR. We can either do this with a soft change (future behavior warning), or a hard change with only versionchanged documentation.

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