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

Move plt import inside functions that use it #9

Merged

Conversation

Erotemic
Copy link
Contributor

@Erotemic Erotemic commented Dec 9, 2021

I'm interested in using this package in a production system.

Overall, it is very well structured and easy to use. There is one tiny issue: matplotlib.pyplot has import-time side effects. There is a lot to say here, but the upshot is that you need to ensure you have all matplotlib backend 100% configured before you import pyplot, otherwise it can lead to unexpected results.

The matplotlib library itself doesn't have this issue, so it's safe to import that at the top-level, but pyplot is another story. A reasonable workaround is to simply delay the import of pyplot until you actually need it. That gives any external system a change to import all of it's main libraries and then do any user-specific backend customization before any plots are generated.

This is all to say: I moved the import matplotlib.pyplot as plt imports into the functions that used them.

Let me know what you think / if there are any issues that you foresee in making this change.

@jack89roberts jack89roberts merged commit e07b9bb into alan-turing-institute:master Dec 14, 2021
@jack89roberts
Copy link
Collaborator

Happy with this one too 👍

jack89roberts added a commit that referenced this pull request Dec 14, 2021
Main changes:
- #8 - add top level API and refactor setup.py/__init__.py
- #9 - only import pyplot when needed
- #10 - make it possible to set random state
- #11 - ensure constants are floats
- #12 - docs now build from main branch
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.

None yet

2 participants