search

lanius / tinyik

A tiny inverse kinematics solver
MIT License
120 stars 21 forks source link

Why bypass `ImportError` in `visualizer.py` #10

Closed walchko closed 4 years ago

walchko commented 4 years ago

Really great work!

https://github.com/lanius/tinyik/blob/e32a25ed48ad0ef820a72f3aee0139ad1625122d/tinyik/visualizer.py#L3-L6

Why do this? Initially, I didn't install with pip install tinyik[viz] and was confused what o3d was when I got an error about not begin able to import it. Suggest removing this since you are bypassing the check and potentially leading to a confusing error for the user.

lanius commented 4 years ago

Thanks you so much! I apologize for confusing you with the error.

The visualizer is not a core feature of tinyik. In order to keep the library as "tiny" as possible, it is set to extra.

If you install tinyik with no extra, open3d will not be installed and you will get the import error in the following facade module, so I'm bypassing the check to avoid it.

https://github.com/lanius/tinyik/blob/e32a25ed48ad0ef820a72f3aee0139ad1625122d/tinyik/__init__.py#L10

That said, I think you're right that this will confuse users who use the library exploratively. Is there any good way to solve this? Shouldn't we be using extra for this purpose in the first place?

walchko commented 4 years ago

I haven't figured out a good way to solve this either in anything I have written. I generally tend to let it fail like this:

try:
    import open3d as o3d
except ImportError:
    print("Please do 'pip install open3d' to use this feature")
    raise

I don't totally like this, but haven't found another clean way to do it. It alerts the user to the missing library.

The next issue you have is the from .visualizer import visualize will issue an error based on what I just suggested. So the answer is to remove the line from __init__.py. However now that forces the user to do something like from tinyik.visualize import Visualizer, which is different than the rest of the other workflow in your module. But this is typically what I have done.

Sorry for the rambling, but I don't have a good answer that I personally like. I think python didn't think through this very well and forces you to do a bunch of try/except statements which makes the code overly complex in places.