Skip to content

Breaking: Have plot methods return plt.Axes object, not matplotlib module #3237

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

Merged
merged 27 commits into from
Aug 17, 2023

Conversation

janosh
Copy link
Member

@janosh janosh commented Aug 13, 2023

Closes #3138.

pymatgen plotting methods using the matplotlib backend return the matplotlib.pyplot module (plt), rather than an object related to the plot itself, such as the Figure or Axes. This is suboptimal API design and inconsistent with the plotly backend which returns go.Figure.

This PR changes all plotting methods to return a plt.Axes object (same as pymatviz) instead of plt.

Reasons:

  • More Intuitive as Axes actually contains the plot attributes (lines, legend, labels, etc.), and is more in line with what users may expect and what they typically interact with when customizing plots.
  • Consistency with pymatviz and pymatgen's own plotly backend.

@janosh janosh added data viz PRs and issues about pymatgen plotting functionality breaking Breaking change api Application programming interface ux User experience labels Aug 13, 2023
janosh added 19 commits August 13, 2023 13:12
E           AttributeError: 'Axes' object has no attribute 'subplot'

/home/runner/work/pymatgen/pymatgen/pymatgen/electronic_structure/plotter.py:1082
E           AttributeError: 'Axes' object has no attribute 'ylim'

/home/runner/work/pymatgen/pymatgen/pymatgen/electronic_structure/plotter.py:3834
E       AttributeError: 'Axes' object has no attribute 'gca'

/home/runner/work/pymatgen/pymatgen/pymatgen/electronic_structure/plotter.py:360:
E           AttributeError: 'Axes' object has no attribute 'ylim'

/home/runner/work/pymatgen/pymatgen/pymatgen/electronic_structure/plotter.py:3838: AttributeError
E           AttributeError: 'Axes' object has no attribute 'xlim'

pymatgen/electronic_structure/plotter.py:3836: AttributeError
E       AttributeError: 'Axes' object has no attribute 'gca'

/home/runner/work/pymatgen/pymatgen/pymatgen/phonon/plotter.py:266: AttributeError
E       AttributeError: 'Axes' object has no attribute 'setp'

pymatgen/electronic_structure/plotter.py:3875: AttributeError
@janosh janosh changed the title Plot methods return plt.Axes object, not plt module Breaking: Have plot methods return plt.Axes object, not matplotlib module Aug 17, 2023
janosh added 4 commits August 17, 2023 14:27
    def test_plot(self):
        # Disabling latex for testing.
        from matplotlib import axes, rc

        rc("text", usetex=False)
        self.plotter.add_dos("Total", self.dos)
        self.plotter.get_plot(units="mev")
        self.plotter_nostack.add_dos("Total", self.dos)
        plt = self.plotter_nostack.get_plot(units="mev")

>       ax = plt.gca()

also attempt fix get_elt_projected_plots
janosh added 2 commits August 17, 2023 15:18
>       assert plt.ylim() == (-4.0, 7.6348), "wrong ylim"

and

>       ax.set_title(ax.get_title(), size=width * 4)
E       AttributeError: 'numpy.ndarray' object has no attribute 'set_title'
@janosh janosh merged commit 5abfb63 into master Aug 17, 2023
@janosh janosh deleted the return-plt-axes-not-plt branch August 17, 2023 22:35
@fraricci
Copy link
Contributor

fraricci commented Nov 2, 2023

If I can chime in here, I noticed a bug potentially introduced by this pull request.
As mentioned here, there is a "ax =" missing here

@janosh
Copy link
Member Author

janosh commented Nov 2, 2023

@fraricci Thanks for reporting and troubleshooting!

DanielYang59 added a commit to DanielYang59/pymatgen that referenced this pull request Aug 9, 2024
DanielYang59 added a commit to DanielYang59/pymatgen that referenced this pull request Sep 6, 2024
janosh added a commit that referenced this pull request Sep 6, 2024
* remove unused read_cube import

* tweak test.yml comments

* try to install BoltzTraP

* fix decompress command

* make installation script modular

* try to modify PATH, test split 4

* fix boltztrap unit test

* use rtol

* properly skip unit test

* fix fig.xticks to ax migration in #3237

* ax.gca -> plg.gca()

* properly use ScalarFormatter

* NEED CONFIRM: fix number of lines

* NEED CONFIRM: fix unit test values

* try to install open babel

* NEED CONFIRM: include fdint

* Revert "NEED CONFIRM: include fdint"

This reverts commit 6b19836.

* skip tests if no fdint

* try to install Vampire

* make vampire executable

* relocate attribute to class docstring

* try to use absolute path in PATH

* use VAMPEXE

* mark vampire test for fix and skip for now

* restore conda openbabel install

* use openbabel-wheel instead

* make download and decompress less verbose

* make module level var all capital

* re-enable matgl tests

* add TODO tag for skipped unit test

* bump matgl version

* bump matgl in optional

* pin torch version in CI

* add reference url

* clarify comment

* remove unused guarded matgl import

* relocate import

* update requirements.txt

* bump openbabel version

* bump openbabel-wheel to latest release 19

* remove matplotlib != 3.9.1 pin as it has been yanked

* remove linux only pin for openbabel-wheel

* add more comment about bolztrap2

* skip openbabel for windows for now

* add TODO tag

* make module var all cap

* rename non test function to avoid test_xxx pattern

* install tblite thru conda-forge

* sort conda forge pack order alphabetically

* fix io.qchem test dir path

* add separate dir var for new qchem files

* TO FIX: enable qchem test

* I tried but failed, need someone smarter than me to fix this

* remove non-win pin for openbabel-wheel

* skip failing openbabel tests for Windows

* skip left out tests for openbabel on win

* uncomment hiphive

* use numpy recommended 2.0.0rc1

* bumpy build sys numpy to 2.1.0

* fall back to numpy 2.0.1 as 2.1.0 don't support python39

* mark test_zsl as to be fixed in this PR

* use float array as per fast_norm require

* reverse numba version bump

* drop upgrade arg for pip install torch

* instead of hinting, just handle

* re-raise from previous error

* make ubuntu dep install inline

* fix test skip and make module var all cap

* add TODO tags

* renames

* Revert "properly use ScalarFormatter"

This reverts commit e2ff5fa.

* Revert "ax.gca -> plg.gca()"

This reverts commit 1d104cb.

* Revert "fix fig.xticks to ax migration in #3237"

This reverts commit a8d04f9.

* Revert "NEED CONFIRM: fix number of lines"

This reverts commit eb7978d.

* Revert "NEED CONFIRM: fix unit test values"

This reverts commit f6a3fc7.

* ruff fix

* skip entire TestBoltztrapPlotter

* use platform.system() == "Windows" over os.name == "nt"

* use platform.system() == "Windows" over  sys.platform == "win32"

---------

Co-authored-by: Janosh Riebesell <janosh.riebesell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Application programming interface breaking Breaking change data viz PRs and issues about pymatgen plotting functionality ux User experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

returning matplotlib.pyplot module in plotting methods
2 participants