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

Split package into three ones: rotation, translation and transform_managers. #236

Open
szobov opened this issue Mar 23, 2023 · 9 comments
Open

Comments

@szobov
Copy link

szobov commented Mar 23, 2023

Hi there,
Thanks a lot for this amazing project. It's great!

I want to bring it to more widespread adoption and adopt it for internal projects.
But the showstopper I faced was that the project required some significant dependencies: "scipy", "lxml", "beautifulsoup4", which are used only in transform_manager's related code.

I think it can bring significant benefits if we split the project into three separated:

  1. transformations, with dependencies: numpy and soft dependency matplotlib
  2. rotations, with dependencies: numpy and soft dependency matplotlib
  3. transform_managers: with dependencies: transformations, rotations and all other dependencies.

It will enable users to install required packages selectively if they don't need to use transform_managers.
If we agree on it, I can implement all required code changes.
I am eager to hear your opinion on it.

Thanks in advance,
Sergei

@AlexanderFabisch
Copy link
Member

Hi @szobov ,

Thanks for your feedback!

You are right, there are too many non-optional dependencies right now. The code is written in a way that you should still be able to use most components without installing scipy or beautifulsoup4. A quick solution for the issue is to install it via pip with --no-deps and install numpy and matplotlib manually.

I'd like to avoid splitting the package into three separate packages. That would break a lot of existing code. I'd rather suggest making scipy and beautifulsoup4 optional dependencies and raise helfpful errors when an optional dependency is missing. I think the best solution would be to select an appropriate set of dependencies like this in pip:

pip install pytransform3d[transform_manager]
pip install pytransform3d[visualizer]
pip install pytransform3d[transform_manager,visualizer]

etc.

@AlexanderFabisch
Copy link
Member

Here is the list of dependencies (including latest features that are under development):

required

  • numpy

plotting

  • matplotlib

plotting of meshes

  • trimesh

visualizer

  • open3d

transform_manager

  • scipy

graph visualization

  • pydot

urdf

  • scipy
  • beautifulsoup4
  • lxml

jacobians

  • scipy

uncertainty

  • scipy

documentation

  • numpydoc
  • sphinx
  • sphinx-gallery
  • sphinx-bootstrap-theme

test

  • pytest
  • pytest-cov

@szobov
Copy link
Author

szobov commented Mar 23, 2023

Thanks for a quick reply @AlexanderFabisch !
Sure, we can do it this way.
If we agree on it, I can try to implement those changes.
What do you think?

@AlexanderFabisch
Copy link
Member

Yes, that would be very helpful! I'd suggest to update the setup.py and documentation accordingly. Also maybe it is worth to take a look at how other Python packages manage their optional dependencies. pandas has lots of those for example. I'm happy to review any pull requests.

@AlexanderFabisch
Copy link
Member

Release 3.0.0 is about to be released soon. Should this be part of it?

@szobov
Copy link
Author

szobov commented Mar 29, 2023

Release 3.0.0 is about to be released soon. Should this be part of it?

Hi @AlexanderFabisch ,
I'm sorry, but I got covid, and I don't think you should rely on me to wait for a release.
Another option could be if you'll let me know the planned date so I can estimate whether I can be on time.
I appreciate your understanding.

@AlexanderFabisch
Copy link
Member

I won't release it before next week. Get well soon!

@szobov
Copy link
Author

szobov commented Apr 18, 2023

Hey @AlexanderFabisch
I'm sorry for the delayed reply.
I recently started working on the implementation.
I decided on implementing a pandas approach, but with a bit of a change to decouple version setting. (I was not happy with writing package names in many places)
I'll make a draft PR soon so that we can discuss it in review.
Thanks for your patience.

@AlexanderFabisch
Copy link
Member

No worries, there is no time pressure :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants