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

Moving import of visualization library inside of function #181

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

senickel
Copy link

@senickel senickel commented May 3, 2024

This will prevent the installation of additional packages on linux when not using visualizations (e.g., when working running code in a container).

Copy link
Owner

@jonnymaserati jonnymaserati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@senickel thanks for the request and good spot... I've gotten a bit lazy recently and appreciate your feedback!

Suggest you review the PEP 8 guidelines on imports here. I think a nicer way to handle this would be to import the PLOTLY variable from visual and then:

if PLOTLY:
    return figure(self, type, **kwargs)
else:
    return None

Or perhaps print("Please pip install plotly to enable figures") or something similar to prompt this dependency be installed?

Copy link
Owner

@jonnymaserati jonnymaserati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think you need to update your .gitignore file - these files should be in the repo.

Copy link
Owner

@jonnymaserati jonnymaserati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as previous - suggest from .visual import PLOTLY and to conditionally return the fig.

Copy link
Owner

@jonnymaserati jonnymaserati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here, make it conditional on visual.PLOTLY.

Copy link
Owner

@jonnymaserati jonnymaserati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! Good spot @nicolaphee. I've been flip flopping between vedo and vtk and was too lazy to add this. This needs cascading through the module though.

Copy link
Owner

@jonnymaserati jonnymaserati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How to nicely handle when VEDO is False?

Copy link
Owner

@jonnymaserati jonnymaserati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How to manage if not VTK?

Copy link
Owner

@jonnymaserati jonnymaserati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff, but it would be nice to inform the user that for this functionality pip install vtk, either when the library is initiated or when the class is called?

@jonnymaserati
Copy link
Owner

Don't forget to bump the __version__ number in .version.

…of github.com:t60digital/welleng into senickel/move-visualization-import-inside-of-function
If the library is not installed other parts of the module should be executeable. However, when a user tries to execute part of the module that depends on the specific library and it is not installed, an ImportError is raised.
@senickel
Copy link
Author

Hi @jonnymaserati!
I finally have some time to review your comments. Thanks a lot for responding and for the great library as well!

Somehow your comments are not referencing any particular code lines on my side. Hence, where I wasn't sure what the comment was about I added 👀

I also changed visual.py to raise an ImportError if the vtk or vedo are not installed but are needed to execute something.

Let me know what you think!

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

Successfully merging this pull request may close these issues.

3 participants