Add file_version property to XML writers#8190
Add file_version property to XML writers#8190tkoyama010 wants to merge 12 commits intomainpyvista/pyvista:mainfrom feat/add-setfileversion-optionpyvista/pyvista:feat/add-setfileversion-optionCopy head branch name to clipboard
Conversation
This adds a file_version property to the _XMLWriter base class that allows users to get and set the VTK XML file format version (major and minor). The property includes proper version checking and gracefully handles cases where the underlying VTK methods (SetDataSetMajorVersion and SetDataSetMinorVersion) are not available in the VTK version being used. Returns (None, None) when getting the version if methods are not available, and raises AttributeError with a helpful message when trying to set the version without VTK support.
Add comprehensive tests for the file_version property in XMLPolyDataWriter: - Test normal cases: setting valid file_version tuples like (2, 2) and (1, 0) - Test validation: verify proper exceptions for invalid inputs - TypeError for non-tuple inputs (list) - ValueError for wrong tuple length - TypeError for non-int values in tuple - Gracefully skip tests when VTK doesn't support file_version methods The test uses pytest.parametrize to test multiple invalid input scenarios and properly handles VTK builds that don't have SetDataSetMajorVersion/ SetDataSetMinorVersion methods.
There was a problem hiding this comment.
Pull request overview
This PR adds a file_version property to the _XMLWriter base class, enabling users to get and set the VTK XML file format version (major and minor version numbers). This provides control over which VTK XML file format features and compression methods are available when writing files.
- Added property getter/setter with comprehensive input validation
- Gracefully handles older VTK versions that don't support file version methods
- Includes extensive tests for both valid inputs and validation of various invalid input types
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| pyvista/core/utilities/writer.py | Implements file_version property in _XMLWriter base class with getter, setter, validation logic, and VTK version compatibility handling |
| tests/core/test_utilities.py | Adds parametrized test covering valid file version values and validation of multiple invalid input types |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Fix mypy error: change type annotation from tuple[int, int] to tuple[int, ...] to allow runtime validation of tuple length - Move pyvista import out of TYPE_CHECKING block since it's used at runtime - Fix pytest.parametrize to use tuple for parameter names - Apply code formatting fixes (ruff, trailing whitespace)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8190 +/- ##
==========================================
- Coverage 96.62% 96.60% -0.03%
==========================================
Files 161 161
Lines 34908 34925 +17
Branches 4418 4419 +1
==========================================
+ Hits 33729 33738 +9
- Misses 574 581 +7
- Partials 605 606 +1 |
user27182
left a comment
There was a problem hiding this comment.
Seems like this feature is a bit broken and isn't currently working as expected.
| >>> writer.file_version = (2, 2) # doctest:+SKIP | ||
| >>> writer.file_version # doctest:+SKIP |
There was a problem hiding this comment.
why skip? When I run this example locally with VTK 9.6, I get (None, None), seems something isn't working?
import pyvista as pv
writer = pv.XMLPolyDataWriter('sphere.vtp', pv.Sphere())
print(writer.file_version) # (None, None)
writer.writer.GetDataSetMajorVersion() # AttributeError| hasattr(self.writer, 'SetDataSetMajorVersion') | ||
| and hasattr(self.writer, 'SetDataSetMinorVersion') | ||
| ): # pragma: no cover | ||
| msg = ( |
There was a problem hiding this comment.
Why no cover? We should have coverage for this automatically for both older vtk (< 9.3) and newer if it's only missing for < 9.3.
Recommend also using pv.vtk_version_info < 9.3 instead, since that will automatically raise an error when < 9.3 is no longer support and hence will help us remove this version branch later.
| @file_version.setter | ||
| def file_version(self, version: tuple[int, ...]) -> None: |
There was a problem hiding this comment.
Seems odd to me that users can set this to any arbitrary value. Surely this should be restricted to specific values supported by VTK? Otherwise it seems users can set version to (42, 99) if they want?
Moreover, looking at the class members for "S" https://vtk.org/doc/nightly/html/functions_s.html#index_s, we can see that only the hyper tree grid writer supports this, so this setter basically doesn't work at all for any of our writers.
| try: | ||
| writer.file_version = (2, 2) | ||
| assert writer.file_version == (2, 2) | ||
| writer.file_version = (1, 0) | ||
| assert writer.file_version == (1, 0) | ||
| except AttributeError: | ||
| # Skip if VTK version doesn't support file_version setter |
There was a problem hiding this comment.
When I run this test locally with VTK 9.6, everything is skipped. This would explain the missing coverage.
We should not unconditionally skip this test on attribute error. We should instead use @pytest.mark.needs_vtk_version(9,3,0), and then test the attribute error being raised with a separate test.
Or, include vtk version-specific branches inside the test.
Pull request was converted to draft
Overview
This PR adds a
file_versionproperty to XML writers that allows users to get and set the VTK XML file format version (major and minor).Different VTK XML file versions may support different features or compression methods. This property provides a convenient way to control the file version when writing XML files.
Details
file_versionproperty (getter and setter) to_XMLWriterbase class(None, None)for getter, raisesAttributeErrorfor setter)