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.

Sign up for GitHub

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

Jump to bottom

Self-import refactor #194

Merged
merged 10 commits into from
Aug 18, 2024
Merged

Self-import refactor #194

merged 10 commits into from
Aug 18, 2024

Conversation

janosh
Copy link
Owner

@janosh janosh commented Aug 17, 2024

import pymatviz as namespace

@janosh janosh added housekeeping Keep the code base tidy. pkg Package labels Aug 17, 2024
@janosh
Copy link
Owner Author

janosh commented Aug 17, 2024

@DanielYang59 curious what you think of refactoring imports in this way, i.e. just having import pymatviz as pmv at the top and accessing plot functions and utilities as attributes?

@DanielYang59
Copy link
Collaborator

DanielYang59 commented Aug 18, 2024 •
edited
Loading

Hi @janosh thanks for pinging me. I'm in favor of this way of import:

  1. It kinds of follows the similar pattern as matplotlib so it might be more intuitive for users to get their hands on hopefully, also performance IMO would not be a key concern for plotting packages (if there is any performance impact).
  2. I personally prefer to de-clutter the namespace (especially when some function is used very few times), e.g. in previous code refactoring of pymatgen mostly, I would prefer import math; math.sin/math.cos... over from math import sin/cos... where the latter brings a lot of functions into the local namespace.
  3. I believe we still could keep both ways of import and let user choose whatever they prefer?

janosh added 2 commits August 18, 2024 07:58
@janosh
# Please enter the commit message for your changes. Lines starting
12febfd 
# with '#' will be ignored, and an empty message aborts the commit.
#
# On branch import-refactor
# Your branch is up to date with 'origin/import-refactor'.
#
# Changes to be committed:
#	modified:   examples/dataset_exploration/camd_2022/explore_camd_2022.py
#	modified:   examples/dataset_exploration/matbench/dielectric/explore_dielectric.py
#	modified:   examples/dataset_exploration/matbench/log_g+kvrh/explore_log_g+krvh.py
#	modified:   examples/dataset_exploration/matbench/perovskites/explore_perovskites.py
#	modified:   examples/dataset_exploration/matpes/eda.py
#	modified:   examples/dataset_exploration/wbm/explore_wbm.py
#	modified:   examples/diatomics/plot.py
#	modified:   examples/make_assets/histogram.py
#	modified:   examples/make_assets/ptable/ptable_matplotlib.py
#	modified:   examples/make_assets/ptable/ptable_plotly.py
#	modified:   pymatviz/__init__.py
#	modified:   pymatviz/histogram.py
#	modified:   pymatviz/ptable/ptable_plotly.py
#	modified:   pymatviz/scatter.py
#	modified:   pymatviz/sunburst.py
#	modified:   pymatviz/uncertainty.py
#	modified:   tests/ptable/test_ptable_matplotlib.py
#	modified:   tests/ptable/test_ptable_plotly.py
#	modified:   tests/test_io.py
#	modified:   tests/test_process_data.py
#
@janosh
fix circular import
da33fa7
@janosh janosh merged commit f4f8a30 into main Aug 18, 2024
6 checks passed
@janosh janosh deleted the import-refactor branch August 18, 2024 06:42
@DanielYang59
Copy link
Collaborator

Thanks for the effort. That's a lot of work!

@janosh
Copy link
Owner Author

janosh commented Aug 18, 2024

not that much actually. 😄 writing lots of new tests in #193 was much more work

@DanielYang59
Copy link
Collaborator

I appreciate your effort in both cases :)

@janosh janosh mentioned this pull request Sep 25, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
housekeeping Keep the code base tidy. pkg Package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@janosh @DanielYang59