SINTEF / Splipy

Spline modelling made easy.
GNU General Public License v3.0
102 stars 18 forks source link

Support for nutils #42

Closed VikingScientist closed 6 years ago

VikingScientist commented 6 years ago

This is built upon #39 and is thus sharing many of the same commits. Merge #39 before this one.

VikingScientist commented 6 years ago
from splipy import *                                                           
from splipy.IO import *                                                        
import splipy.surface_factory as sf                                            
from splipy.utils import image               

# read single closed curve from image 
crv = image.image_curves('texas.png')[0]  
# look for natural corners in geometry
breakpts = crv.get_kinks() 
# split closed curve into 4 parts          
four_crvs = crv.split([breakpts[0], 0.2, breakpts[1], breakpts[3]])  

# change type to try 'poisson' or 'elasticity' ('coons' was the default behaviour before)
srf = sf.edge_curves(four_crvs, type='finitestrain')  

with G2("finalout.g2") as f:                                                   
    f.write(srf)                                                               

Texas.png texas

finalout.g2 texas

This particular example is notoriously difficult due to the interior 270 degree corner and both 'poisson', 'elasiticity' and 'coons' will all produce self-intersecting geometries for this set of input curves.

TheBB commented 6 years ago

Well done. :+1:

Maybe the new functions that are very similar can be reorganized, e.g. move surface_factory.py to surface_factory/__init__.py and then put the new functions in surface_factory/whatever.py, and import them from there.

This PR also reminds me that we should specify extras_require in our setup.py-file for optional dependencies, which AFAIK at the moment is OpenCV and, with this, nutils. See http://setuptools.readthedocs.io/en/latest/setuptools.html#declaring-extras-optional-features-with-their-own-dependencies

For that matter, it would be good to gracefully report an error if the user is trying to use a feature requiring nutils instead of blindly trying to access an attribute that doesn't exist, leaving the user with an unexplained AttributeError or something like that. It looks like the image module will also just happily blow up in an arbitrary way right now.

VikingScientist commented 6 years ago

So the cleanest way to detect the absence of packages is by try except-statements, and the cleanest way of alerting the user of something fishy is by raising exceptions. This lead me to the following code

def poisson_patch(bottom, right, top, left):                                   
    try:                                                                       
        from nutils import mesh, function as fn                                
        from nutils import _, log, library                                     
    except ImportError as e:                                                   
        raise ImportError(e.message + 'FEM solvers not available: try installing with "pip install nutils"')

However, python 3 is not a fan of exception-handling raising exceptions of their own. Moreover, this behaviour is different in python 2, resulting in libraries such as six which offers compatability-functions for both python 2 and 3 (because six=2*3, very clever I know :smile: ). This library does contain a reraise function, but at this point I don't know if this is neccessary the right way to go about it. Would it not be enough to let poisson_patch just throw the default ImportError that appears and assume that the user knows enough python that this is fixed by installing the missing library, preferably through pip. It is listed as an optional dependency on the front page and the ImportError does already mention nutils.

After writing this up, then I am confident that this is the right approach and a commit will arrive shortly. The change is then to move the import commands to the function using them, instead of silently ignoring it at the top of the file. For comparison, here is the error messages produced by

Importing at the top (with in-place exception handling)

    domain, geom = mesh.rectilinear([k1, k2])
UnboundLocalError: local variable 'mesh' referenced before assignment

Importing at the function, with no exception handling

    from nutils import mesh, function as fn
ImportError: No module named 'nutils'
TheBB commented 6 years ago

Yes, that's a lot better. I was thinking something like this. Admittedly a little hacky but…

from functools import wraps

def require_nutils(func):
    try:
        import nutils
        func.__globals__['nutils'] = nutils
        return func
    except ImportError:
        @wraps(func)
        def inner(*args, **kwargs):
            raise ImportError("Using this feature requires nutils to be installed.")
        return inner

@require_nutils
def test():
    print(nutils)

test()
VikingScientist commented 6 years ago

Any comments on this? Could we merge?

TheBB commented 6 years ago

Yeah, fine.