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

Prefer pymatviz interactive plotly version of periodic table heatmap if available #3180

Merged
merged 4 commits into from
Jul 25, 2023

Conversation

janosh
Copy link
Member

@janosh janosh commented Jul 24, 2023

This PR changes periodic_table_heatmap() which now tries to import ptable_heatmap_plotly from pymatviz. If successful, all arguments are passed to that function along with deprecation messages for args that have different names in pymatviz.ptable_heatmap_plotly vs pymatgen.util.plotting.periodic_table_heatmap.

It does not add pymatviz as a new dependency of pymatgen to avoid circularity.

Before

pmg-ptable-heatmap

After
ptable-heatmap-plotly-more-hover-data

Code

import pandas as pd
from pymatgen.util.plotting import periodic_table_heatmap

df_ptable = pd.read_csv(
    "https://github.com/janosh/pymatviz/raw/main/pymatviz/elements.csv", comment="#"
).set_index("symbol")

periodic_table_heatmap(df_ptable.atomic_mass.to_dict(), cmap="viridis")

@janosh janosh added enhancement A new feature or improvement to an existing one data viz PRs and issues about pymatgen plotting functionality labels Jul 24, 2023
@mkhorton
Copy link
Member

Why keep both, if we’re referring people to functionality in pymatviz? We should probably have a strategy here.

Similar issue with plotting functionality in crystal toolkit; generally, plotting changes there have been upstreamed to pymatgen, but only for plotters that already existed within pymatgen, because I didn’t want two variants of, say, PDPlotter in existence. Not all functionality has been moved yet, there are still Pourbaix plotting enhancements for example.

@janosh
Copy link
Member Author

janosh commented Jul 24, 2023

@mkhorton That's the long-term plan. This is meant as the 1st step in a deprecation/migration phase. But happy to discuss other options.

@shyuep
Copy link
Member

shyuep commented Jul 24, 2023

I would just note this - there will always be newer better packages coming up. There are two outcomes: 1. pymatgen depends on the new package for some functionality. or 2. that new package gets merged into pymatgen.

For (1) to be accepted by me, the package has to deliver major functionality, have reasonable long-term maintenance, and does not depend on pymatgen itself. Something like numpy satisfies all three criteria. mp-api fails the last criterion.

Right now, pymatviz fails at least one, if not all three criteria (it depends on pymatgen, and while I have confidence in Janosh's coding, pymatviz is not part of some long-term funded infrastructure).

If (1) or (2) are not acceptable to either pymatgen maintainers or the author of the package in question, then the only other outcome is that it is an optional functionality that is used when present. That is what this PR is.

@janosh
Copy link
Member Author

janosh commented Jul 24, 2023

@mkhorton Should I leave this PR open until further brainstorming?

@janosh janosh enabled auto-merge (squash) July 25, 2023 18:35
@janosh janosh merged commit 10d5cc5 into master Jul 25, 2023
27 checks passed
@janosh janosh deleted the pymatviz-ptable-heatmap branch July 25, 2023 18:52
@shyuep
Copy link
Member

shyuep commented Jul 25, 2023

You can merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data viz PRs and issues about pymatgen plotting functionality enhancement A new feature or improvement to an existing one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants