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

Set VTK 9.0 as the minimum supported version #4018

Merged
merged 7 commits into from
Mar 7, 2023

Conversation

mwtoews
Copy link
Contributor

@mwtoews mwtoews commented Feb 14, 2023

Overview

As discussed in #3938 this PR removes pre-VTK 9 logic, setting VTK 9.0 as the minimum supported version.

Details

  • Plenty of chops and simplifications where pre-VTK 9 logic was enabled
  • Changes to pyvista/_vtk.py are difficult to see due the change of indent level, but are relatively minor
  • Keep pyvista._vtk.VTK9 for now, as this is used externally by pytest-pyvista; remove pyvista._vtk.VTK91 which had limited use
  • From pyvista/utilities/cells.py, remove generate_cell_offsets_loop() and generate_cell_offsets(), which were only used for pre-VTK9 UnstructuredGrid() with four arguments
  • With the plotting renderer classess, remove the can_process_events property and other variables like VERY_FIRST_RENDER and update_timer_id
  • Update docstrings and examples, where relevant
  • Remove comments, notes or other remarks about previous versions
  • Removed the needs_vtk9 pytest marker

@github-actions github-actions bot added documentation Anything related to the documentation/website maintenance Low-impact maintenance activity labels Feb 14, 2023
@codecov
Copy link

codecov bot commented Feb 14, 2023

Codecov Report

Merging #4018 (656dc39) into main (9b24a21) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #4018      +/-   ##
==========================================
+ Coverage   94.51%   94.52%   +0.01%     
==========================================
  Files          97       97              
  Lines       20609    20464     -145     
  Branches     3480     3430      -50     
==========================================
- Hits        19478    19344     -134     
+ Misses       1130     1119      -11     
  Partials        1        1              

@MatthewFlamm
Copy link
Contributor

I started pyvista/pytest-pyvista#44 to enable this PR.

pyproject.toml Outdated Show resolved Hide resolved
@banesullivan
Copy link
Member

This is fantastic and I am eager to merge this when everyone is on board!

Copy link
Contributor

@MatthewFlamm MatthewFlamm left a comment

Choose a reason for hiding this comment

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

Some comments to get started.

pyvista/_vtk.py Outdated Show resolved Hide resolved
pyvista/utilities/reader.py Show resolved Hide resolved
tests/test_filters.py Outdated Show resolved Hide resolved
@tkoyama010 tkoyama010 mentioned this pull request Feb 27, 2023
2 tasks
@MatthewFlamm
Copy link
Contributor

This PR will be increasingly hard to maintain as other PRs are merged. I recommend merging soon if agreed upon. I made pyvista/pytest-pyvista#47 for a release then this PR can be finalized.

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.

This is good to go. Thanks @mwtoews.

Let's merge this soon to avoid any potential merge conflicts.

Copy link
Contributor

@MatthewFlamm MatthewFlamm left a comment

Choose a reason for hiding this comment

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

A forward looking thought: I'm not sure whether it would be now better to from vtkmodules import ... where it is used. _vtk now mostly just flattens vtkmodules.

@akaszynski
Copy link
Member

A forward looking thought: I'm not sure whether it would be now better to from vtkmodules import ... where it is used. _vtk now mostly just flattens vtkmodules.

This is a great point. One issue we've had is that we load everything all upfront despite not using all libraries. If we instead "lazy load" everything, we can improve load time and memory footprint.

@akaszynski akaszynski merged commit f929695 into pyvista:main Mar 7, 2023
@mwtoews mwtoews deleted the set-vtk9-as-min branch March 7, 2023 20:53
@akaszynski akaszynski mentioned this pull request Mar 8, 2023
@akaszynski akaszynski mentioned this pull request Apr 30, 2023
6 tasks
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 maintenance Low-impact maintenance activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants