Repository: pyvista/pyiges (language: python)

Issue:
Request if there is interest to detach pyiges from vtk
Hello,

I'd like to know if You would be interested in decoupling `pyiges` from `vtk` into a pure parsing library?

As there is no other comparable python library for IGES parsing, I think modularity could lead to more people using it, which then could also contribute.

I needed exactly this and already stripped the VTK-part away for me, but if there is interest, I could provide it and help in integrating the pure-parsing code to `pyvista/pyiges`.

**Roadmap**

- [x] increase Test coverage from given example data to 85%, see #30 
- [ ] increase Test coverage with new example data  to 100%, see #31 
- [x] decouple pure parser as specified in https://github.com/pyvista/pyiges/issues/28#issuecomment-1499524376


Discussion / hints:
I think it'd be pretty easy to make the PyVista (VTK) dependency soft and raise an `ImportError` when not available. If you've already done this, definitely open a Pull Request, and I bet we could integrate it
Dear Bane,

Thanks for Your reply :)

In my case, the changes are a bit more rigorous. The main aspect is, that I basically removed all `to_vtk`-Methods from all the `Iges`-Classes.

I think that from the perspective of a pure standalone parsing module, it makes sense to just represent the parsed data, while the `to_vtk` should be functionality outside of this module, as it has a sharper context, as would also any other evaluation/transformation functionality.

For the pyiges, the changes would therefore be a bit larger, basically as follows:
- there is a pure parsing module `pyiges_parse` (= `pyvista/pyiges(old)` minus vtk-code),
- there is `pyvista/pyiges(new)` which imports `pyiges_parse` and adds the `to_vtk`-functionality, the easiest way would be IMO by subclassing each `pyiges_parse.Iges`-Subclass to add the proper `to_vtk`-method.

```python
class RationalBSplineCurve(pyiges_parse.RationalBSplineCurve):

    def to_vtk(self, ...):
        ...
```

Do You think this would be too much effort from Your side? I could imagine trying to contribute that and alsocould create a  deeper first sketch of the design changes as a base for discussion, but it would be important to know first, if You are basically interested to go into this direction.
Simply having a check that raises an `ImportError` when VTK/PyVista is missing would be much simpler and easier to maintain with the exact same effect

I do not think we could buy into splitting this into separate packages -- this would add too much complexity and maintenance burden.

To be clear, I have nothing to do with this repo and simply help maintain many of the "side projects" like this included in the PyVista org -- going to defer to @akaszynski for any final say here


---- 

If I were to decouple the PyVista dependency, I'd implement a decorator for all of the methods that use PyVista to perform a runtime check:

```py
try:
    import pyvista
except:
    pyvista = None

def _check_pyvista(func):
    def wrapper(*args, **kwargs):
        if pyvista is None:
            raise ImportError('Please install PyVista')
        return func(*args, **kwargs)
    return wrapper

...

class IgesStuff:
    ...
    @_check_pyvista  # add this decorator to methods that use pyvista/vtk
    def to_pyvista(self, *args, **kwargs):
```

Then remove PyVista/VTK from the core dependencies and done.

Another approach would be to split these methods into a mixin-class that only gets mixed in if PyVista/VTK is available, but I don't like this approach as it would mean the API itself could dynamically change and lead to unexpected AttributeErrors for users
Thanks, I understand Your concerns. Say, if I'd provide the code work on both sides `pyiges_parse` and `pyiges` (I'd again have to double check if the approach fully works out..., yet I only did the simpler first part), including more Tests to make it trustworthy, such that only the reviewing and the future maintenance were on Your side, would it still be too much work for You?
That seems reasonable to me if you're up for it!! Let's just keep it all in this repository, though. @akaszynski, any final thoughts?
Totally fine with that. Please ping me for review. A bit underwater with notifications.
Thank You.

I'd be fine with staying in the repository, but as far as I understand, the independence of `vtk` as a real effort to the world outside `pyvista` would only be accomplished if `pyiges_parse` becomes installable without `vtk`, which is my motivation.

Following my (not very educated) understanding of modules, this would be accomplishable by either splitting `pyiges(old)` in into two modules `pyiges_parse` and `pyiges(new)` or having a single module with a soft dependency on `vtk`.

Both approaches have downsides:
- Single-module approach: The only way I know to implement the soft dependency is by defining `extras_require` in `setup.py` with "plugins" for `geomdl` and `vtk`. To install the pure parsing package, one would install `pyiges`, while, to install the "full" package one would need to specify `pyiges[geomdl,vtk]` (or something like `pyiges[full]` joining the two). This would break existing `pip install` scripts, which do not pin their version. `pip install` scripts that pin their version and raise them, would end up having a `vtk,geomdl`-less `pyiges`, which the maintainers would have to see and fix by adding the `[...]` extras .
- two-module-approach: means more code changes in Your history to review and more folder structure logic in the repository. But existing projects depending on the full `pyiges` wouldn't notice anything, while new projects could install the pure parsing package as `pyges_parse` independently.

Do You find one of these approaches still acceptable, or see a third solution? I'd be fine with both preferring the single-module approach.

I'd go with the first approach `pyiges[full]`. Raise an `ImportError` when attempting to access functionality that requires dependencies.
Thank You @akaszynski, I'd call back when I added some tests, it might then make sense to merge them independently before carrying out the decoupling.
@akaszynski @banesullivan As suggested, I added #30 to increase the test coverage separately as a pre step of the decoupling, please let me know if You have any critique.

Fix patch:
diff --git a/README.rst b/README.rst
index c64f151..7d3633b 100644
--- a/README.rst
+++ b/README.rst
@@ -21,11 +21,17 @@ following entities:
 
 Installation
 ------------
+
+``pyiges`` is offered in a "full" variant including the conversion features
+and a "pure" parsing module variant.
+The pure variant has no conversion features, no dependencies to ``pyvista,geomdl``,
+and can be installed by removing the ``[full]`` specificator from the following commands.
+
 Install with pip using:
 
 .. code::
 
-   pip install pyiges
+   pip install pyiges[full]
 
 Otherwise, if you want the bleeding edge version, feel free to clone
 this repo and install with:
@@ -34,7 +40,9 @@ this repo and install with:
 
    git clone https://github.com/pyvista/pyiges
    cd pyiges
-   pip install .
+   pip install .[full]
+
+Note that the square brackets might need to be escaped or quoted when using ``zsh``.
 
 
 Usage
diff --git a/docs/index.rst b/docs/index.rst
index da7a775..08b92e2 100644
--- a/docs/index.rst
+++ b/docs/index.rst
@@ -28,11 +28,17 @@ Contents
 
 Installation
 ------------
+
+``pyiges`` is offered in a "full" variant including the conversion features
+and a "pure" parsing module variant.
+The pure variant has no conversion features, no dependencies to ``pyvista,geomdl``,
+and can be installed by removing the ``[full]`` specificator from the following commands.
+
 Install with pip using:
 
 .. code::
 
-   pip install pyiges
+   pip install pyiges[full]
 
 Otherwise, if you want the bleeding edge version, feel free to clone
 this repo and install with:
@@ -41,7 +47,9 @@ this repo and install with:
 
    git clone https://github.com/pyvista/pyiges
    cd pyiges
-   pip install .
+   pip install .[full]
+
+Note that the square brackets might need to be escaped or quoted when using ``zsh``.
 
 
 Usage
diff --git a/pyiges/check_imports.py b/pyiges/check_imports.py
new file mode 100644
index 0000000..45b2f5e
--- /dev/null
+++ b/pyiges/check_imports.py
@@ -0,0 +1,20 @@
+
+try:
+    import pyvista
+    import geomdl
+    from pyvista._vtk import vtkAppendPolyData
+    _IS_FULL_MODULE = True
+except (ModuleNotFoundError, ImportError) as exc:
+    pyvista = None
+    geomdl = None
+    vtkAppendPolyData = None
+    _IS_FULL_MODULE = False
+    _PROBLEM_MSG = 'Import from \'{}\' failed, to support this feature '\
+        'please install pyiges[full]'.format(exc.name)
+
+def assert_full_module_variant(inner_func):
+    def safe_func(*a, **kw):
+        if not _IS_FULL_MODULE:
+            raise Exception(_PROBLEM_MSG)
+        return inner_func(*a, **kw)
+    return safe_func
diff --git a/pyiges/geometry.py b/pyiges/geometry.py
index 4ec801c..9824628 100644
--- a/pyiges/geometry.py
+++ b/pyiges/geometry.py
@@ -1,8 +1,8 @@
 import os
 
-from geomdl import NURBS, BSpline
+from pyiges.check_imports import assert_full_module_variant, geomdl, pyvista as pv
+
 import numpy as np
-import pyvista as pv
 
 from pyiges.entity import Entity
 
@@ -18,8 +18,6 @@ def parse_float(str_value):
     except ValueError:
         return float(str_value.lower().replace("d", "e"))
 
-
-
 class Point(Entity):
     """IGES Point"""
 
@@ -56,6 +54,7 @@ def __repr__(self):
     def __str__(self):
         return self.__repr__()
 
+    @assert_full_module_variant
     def to_vtk(self):
         """Point represented as a ``pyvista.PolyData`` Mesh
 
@@ -91,6 +90,7 @@ def __repr__(self):
         s += "To point {0}, {1}, {2}".format(self._x2, self._y2, self._z2)
         return s
 
+    @assert_full_module_variant
     def to_vtk(self, resolution=1):
         """Line represented as a ``pyvista.PolyData`` Mesh
 
@@ -154,6 +154,7 @@ def to_affine(self):
                          [self.r31, self.r32, self.r33, self.t3],
                          [0, 0, 0, 1]])
 
+    @assert_full_module_variant
     def _to_vtk(self):
         """Convert to a vtk transformation matrix"""
         vtkmatrix = pv.vtkmatrix_from_array(self.to_affine())
@@ -227,6 +228,7 @@ def __repr__(self):
         info += 'Scalar coefficient:  %f' % self.f
         return info
 
+    @assert_full_module_variant
     def to_vtk(self):
 
         # a*x**2 + b*x*y + c*y**2 + d*x + e*y + f = 0
@@ -305,7 +307,9 @@ def __str__(self):
             s += "Unit normal: {0} {1} {2}".format(self.XNORM, self.YNORM, self.ZNORM)
         return s
 
+    @assert_full_module_variant
     def to_geomdl(self):
+        from geomdl import NURBS
         curve = NURBS.Curve()
         curve.degree = self.M
         curve.ctrlpts = self.control_points
@@ -313,6 +317,7 @@ def to_geomdl(self):
         curve.knotvector = self.T  # Set knot vector
         return curve
 
+    @assert_full_module_variant
     def to_vtk(self, delta=0.01):
         """Set evaluation delta (controls the number of curve points)
         """
@@ -544,8 +549,10 @@ def __repr__(self):
         info += '    Control Points: %d' % len(self._cp)
         return info
 
+    @assert_full_module_variant
     def to_geomdl(self):
         """Return a ``geommdl.BSpline.Surface``"""
+        from geomdl import BSpline
         surf = BSpline.Surface()
 
         # Set degrees
@@ -562,6 +569,7 @@ def to_geomdl(self):
         surf.weights = self._weights
         return surf
 
+    @assert_full_module_variant
     def to_vtk(self, delta=0.025):
         """Return a pyvista.PolyData Mesh
 
@@ -621,6 +629,7 @@ def _add_parameters(self, parameters):
         self.y2 = parse_float(parameters[7])
         self._transform = self.d.get('transform', None)
 
+    @assert_full_module_variant
     def to_vtk(self, resolution=20):
         """Circular arc represented as a ``pyvista.PolyData`` Mesh
 
diff --git a/pyiges/iges.py b/pyiges/iges.py
index 0c7c45c..ec1bff6 100644
--- a/pyiges/iges.py
+++ b/pyiges/iges.py
@@ -1,13 +1,10 @@
 from tqdm import tqdm
+
+from pyiges.check_imports import assert_full_module_variant, pyvista, vtkAppendPolyData
+
 from pyiges.entity import Entity
 from pyiges import geometry
 
-try: 
-    import pyvista 
-    from pyvista._vtk import vtkAppendPolyData
-except ImportError:
-    pass 
-
 class Iges():
     """pyiges.Iges object
 
@@ -43,6 +40,7 @@ def entities(self):
          <pyiges.entity.Entity at 0x7f7056069910>]
         """
 
+    @assert_full_module_variant
     def to_vtk(self, lines=True, bsplines=True,
                surfaces=True, points=True, delta=0.025, merge=True,
                progress=tqdm):
diff --git a/setup.py b/setup.py
index ad3e03c..1acaa3e 100644
--- a/setup.py
+++ b/setup.py
@@ -36,6 +36,9 @@
     ],
     description='Pythonic IGES reader',
     url='https://github.com/pyvista/pyiges',
-    install_requires=['tqdm', 'geomdl', 'pyvista>=0.28.0'],
+    install_requires=['tqdm', 'numpy'],
+    extras_require={
+        'full' : ['geomdl', 'pyvista>=0.28.0']
+    },
     package_data={'pyiges.examples': ['impeller.igs', 'sample.igs']}
 )


Test patch:
diff --git a/.github/workflows/testing-and-deployment.yml b/.github/workflows/testing-and-deployment.yml
index 8e1ebe4..6cfbd0b 100644
--- a/.github/workflows/testing-and-deployment.yml
+++ b/.github/workflows/testing-and-deployment.yml
@@ -46,6 +46,8 @@ jobs:
           pip install -r requirements_test.txt --disable-pip-version-check
           cd tests
           pytest -v --cov pyiges --cov-report xml
+          pip install -e ..[full] --disable-pip-version-check --find-links=dist
+          pytest -v --cov pyiges --cov-report xml --expect-full-module
 
       - name: Upload to PyPi
         if: github.event_name == 'push' && startsWith(github.event.ref, 'refs/tags')
diff --git a/tests/conftest.py b/tests/conftest.py
new file mode 100644
index 0000000..e071efa
--- /dev/null
+++ b/tests/conftest.py
@@ -0,0 +1,3 @@
+
+def pytest_addoption(parser):
+    parser.addoption("--expect-full-module", action='store_true', default=False)
diff --git a/tests/test_pyiges.py b/tests/test_pyiges.py
index d1e384d..be56809 100644
--- a/tests/test_pyiges.py
+++ b/tests/test_pyiges.py
@@ -1,3 +1,4 @@
+import functools
 import numpy as np
 import os
 import pytest
@@ -7,6 +8,20 @@
 
 DIR_TESTS_REFERENCE_DATA = os.path.join(os.path.dirname(__file__), 'reference_data')
 
+def adjust_depending_on_package_variant(test_func):
+    ''' If the full module is present, run test as usual,
+    otherwise assert that the error is triggered as intended.
+    '''
+    @functools.wraps(test_func)
+    def generalized_test_func(*a, **kw):
+        if pyiges.check_imports._IS_FULL_MODULE:
+            return test_func(*a, **kw)
+        else:
+            with pytest.raises(Exception, match=r'install pyiges\[full\]'):
+                return test_func(*a, **kw)
+    return generalized_test_func
+
+
 @pytest.fixture(scope='module')
 def sample():
     return pyiges.read(examples.sample)
@@ -88,17 +103,20 @@ def test_parse_completeness2(sample):
 
 
 @pytest.mark.parametrize('merge', [False, True])
+@adjust_depending_on_package_variant
 def test_iges_to_vtk1(impeller, merge):
     items = impeller.to_vtk(delta=0.5, merge=merge)
     assert items.bounds == pytest.approx((-50.516562808,  49.445007453, -49.510005208,
                                           50.494748866, -997.9630126953125, 1011.0750122070312))
 
 @pytest.mark.parametrize('merge', [False, True])
+@adjust_depending_on_package_variant
 def test_iges_to_vtk2(sample, merge):
     items = sample.to_vtk(merge=merge)
     assert items.bounds == pytest.approx((0.0, 1.0, 0.0, 1.0, 0.0, 0.0))
 
 
+@adjust_depending_on_package_variant
 def test_surfaces_vtk(surf):
     mesh = surf.to_vtk(delta=0.1)
 
@@ -116,6 +134,29 @@ def test_surfaces_vtk(surf):
                                          -9.363301218200274, -45.772995131000016, -8.876323512))
 
 
+@adjust_depending_on_package_variant
+def test_surfaces_to_geomdl(surf):
+    gsurf = surf.to_geomdl()
+    assert gsurf.data['control_points'] == pytest.approx((
+        [-26.902905334, -16.51153913, -8.876323512],
+        [-25.851820351, -15.866440371, -21.167794775],
+        [-25.995725563, -15.954761558, -33.519826531],
+        [-27.332763631, -16.775362758, -45.772995131],
+        [-28.232974775, -14.344404257, -8.876323512],
+        [-27.129924549, -13.783974529, -21.167794775],
+        [-27.28094438, -13.860703585, -33.519826531],
+        [-28.684085103, -14.573601108, -45.772995131],
+        [-29.292803148, -12.033057876, -8.876323512],
+        [-28.148345882, -11.562931461, -21.167794775],
+        [-28.305034796, -11.62729699, -33.519826531],
+        [-29.760847559, -12.225323719, -45.772995131],
+        [-30.067010387, -9.611041892, -8.876323512],
+        [-28.892305175, -9.235542604, -21.167794775],
+        [-29.05313537, -9.286952628, -33.519826531],
+        [-30.547425187, -9.764608432, -45.772995131],
+    ))
+
+
 def test_surfaces_parse(surf):
     assert surf.parameters == []
     assert surf.sequence_number == 3
@@ -230,6 +271,7 @@ def test_bsplines_parse(curve):
     assert str(curve)
 
 
+@adjust_depending_on_package_variant
 def test_bsplines_vtk(curve):
     mesh = curve.to_vtk()
 
@@ -246,7 +288,28 @@ def test_bsplines_vtk(curve):
     assert mesh.bounds == pytest.approx((0.531027642, 0.559345007,
                                          0.006617509, 0.127235606, 0., 0.))
 
+@adjust_depending_on_package_variant
+def test_bsplines_to_geomdl(curve):
+    gcurve = curve.to_geomdl()
+    assert gcurve.data['control_points'] == pytest.approx((
+        [0.531027642, 0.127235606, 0.0, 1.0],
+        [0.53482791, 0.099559574, 0.0, 1.0],
+        [0.537603234, 0.077944022, 0.0, 1.0],
+        [0.539206635, 0.06545597, 0.0, 1.0],
+        [0.540802737, 0.052996393, 0.0, 1.0],
+        [0.542675511, 0.038377045, 0.0, 1.0],
+        [0.544208805, 0.028733039, 0.0, 1.0],
+        [0.546564848, 0.013914155, 0.0, 1.0],
+        [0.548603824, 0.009169564, 0.0, 1.0],
+        [0.551376925, 0.002716702, 0.0, 1.0],
+        [0.554223526, 0.012546793, 0.0, 1.0],
+        [0.556570985, 0.020653212, 0.0, 1.0],
+        [0.557884113, 0.035401282, 0.0, 1.0],
+        [0.55898716, 0.047789894, 0.0, 1.0],
+        [0.559345007, 0.06395869, 0.0, 1.0],
+    ))
 
+@adjust_depending_on_package_variant
 def test_points_vtk(sample):
     points = sample.points(as_vtk=True, merge=True) # pv.PolyData
 
@@ -331,6 +394,7 @@ def test_point_parse(point):
     assert str(point)
 
 
+@adjust_depending_on_package_variant
 def test_point_vtk(point):
     point = point.to_vtk() # pv.PolyData
 
@@ -380,6 +444,7 @@ def test_circular_arcs_parse(carc):
     assert str(carc)
 
 
+@adjust_depending_on_package_variant
 def test_circular_arcs_vtk(carc):
     polydata = carc.to_vtk()
 
@@ -479,6 +544,7 @@ def test_line_parse(line):
     assert str(line)
 
 
+@adjust_depending_on_package_variant
 def test_line_vtk(line):
     polydata = line.to_vtk()
     assert polydata.n_arrays == 2
@@ -533,12 +599,12 @@ def test_transformation_parse(trafo):
     assert repr(trafo)
     assert str(trafo)
 
+@adjust_depending_on_package_variant
 def test_transformation_vtk(trafo):
     trafo = trafo._to_vtk()
     m = trafo.GetMatrix()
     assert m.GetElement(2,3) == pytest.approx(5.67397368511119)
 
-
 def test_example_with_invalid_conic_arc_and_form1_global_line():
     # For this file, the conic arc cannot be parsed.
     # This is either because of a wrong format of the file or a bug in the parsing.
@@ -548,7 +614,7 @@ def test_example_with_invalid_conic_arc_and_form1_global_line():
     assert len(iges.bsplines()) == 1
     assert len(iges.circular_arcs()) == 1
 
-
+@adjust_depending_on_package_variant
 def test_to_vtk(impeller):
     lines = impeller.to_vtk(lines=True, bsplines=False, surfaces=False)
     assert lines.n_points
@@ -575,3 +641,7 @@ def test_parse_separators_from_first_global_line(line, expected_separators):
     else:
         separators = pyiges.Iges._parse_separators_from_first_global_line(line)
         assert separators == expected_separators
+
+def test_package_variant_detection(request):
+    is_full_module_expected = request.config.getoption("--expect-full-module")
+    assert pyiges.check_imports._IS_FULL_MODULE == is_full_module_expected