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

Fix pdplotter.show with matplotlib backend #3493

Merged
merged 5 commits into from
Dec 6, 2023

Conversation

lbluque
Copy link
Contributor

@lbluque lbluque commented Dec 5, 2023

Checklist

  • Google format doc strings added. Check with ruff.
  • Type annotations included. Check with mypy.
  • Tests added for new features/fixes.
  • If applicable, new classes/functions/modules have duecredit @due.dcite decorators to reference relevant papers by DOI (example)

Tip: Install pre-commit hooks to auto-check types and linting before every commit:

pip install -U pre-commit
pre-commit install

Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

Thanks @lbluque! 👍

Can we change this method to return the ax (fig for plotly)? Don't see a reason not to return. And add a test for the return type?

@lbluque
Copy link
Contributor Author

lbluque commented Dec 6, 2023

Thanks @lbluque! 👍

Can we change this method to return the ax (fig for plotly)? Don't see a reason not to return. And add a test for the return type?

That looks like the get_plot method already does. This is for the show method to display the plot, in which the current implementation only works with the plotly backend.

@janosh
Copy link
Member

janosh commented Dec 6, 2023

That looks like the get_plot method already does.

I know but does it hurt to return anyway? I guess it could actually. I think in Jupyter if some code calls plt.show() and you don't assign the returned figure to a variable the cell will render the returned figure and so it would be shown twice? If that's the case then you're right and we shouldn't return.

@janosh janosh added tests Issues with or changes to the pymatgen test suite fix Bug fix PRs data viz PRs and issues about pymatgen plotting functionality analysis Concerning pymatgen.analysis labels Dec 6, 2023
Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

I added a bunch more tests including ones that mock PDPlotter.show() in CI. This is good to go. 🎉

@janosh janosh enabled auto-merge (squash) December 6, 2023 21:53
@janosh janosh merged commit 33beea2 into materialsproject:master Dec 6, 2023
22 checks passed
@lbluque
Copy link
Contributor Author

lbluque commented Dec 7, 2023

Awesome thanks for adding the tests! lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analysis Concerning pymatgen.analysis data viz PRs and issues about pymatgen plotting functionality fix Bug fix PRs tests Issues with or changes to the pymatgen test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants