Add ptable_scatter #122
Add ptable_scatter #122
Conversation
for more information, see https://pre-commit.ci
…tviz into ptable_scatter
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
Please review @janosh . Thanks! |
janosh
added
enhancement
readme.md
Outdated
| | ![ptable-hists] | ![ptable-scatters] | | ||
|
|
||
| [ptable-hists]: https://raw.githubusercontent.com/janosh/pymatviz/main/assets/ptable-hists.svg | ||
| [ptable-scatters]: https://raw.githubusercontent.com/janosh/pymatviz/main/assets/ptable-scatters.svg |
There was a problem hiding this comment.
Sorry this is an overlook from my side, forgot to replace "_" with "-". Meanwhile I thought we're putting all assert svgs together at the end of the readme.md?
No rush at all. Thanks a lot for offering that but let me leave the honour for later (didn't do much in this PR than copied and modified you and @chiang-yuan's |
seems of little use in hindsight
There was a problem hiding this comment.
thanks a lot @DanielYang59! great contribution! 👍
one thing i noticed that could be improved:
following your example in examples/_generate_assets.py, I tried
data_dict = {
ele.symbol: [
xs := np.random.randn(100),
xs + 0.2 * np.random.randn(100),
0.2 * np.random.randn(100),
]
for ele in elements
}
fig = ptable_scatters(
data_dict, colormap="coolwarm", cbar_title="Periodic Table Scatter Plots"
)and got
would prefer the subplots to always be square so tried adding ax.set_aspect("equal") but then i got very small tiles:
do you know of a solution?
tests/test_ptable.py
Outdated
| @@ -318,7 +318,7 @@ def test_ptable_heatmap_plotly_cscale_range( | |||
| fig = ptable_heatmap_plotly(df_ptable.density, cscale_range=cscale_range) | |||
| trace = fig.data[0] | |||
| assert "colorbar" in trace | |||
| # check for correct colorbar range | |||
| # check for correct color bar range | |||
There was a problem hiding this comment.
I think we agreed on using colorbar instead of color bar here...Maybe your spelling checking plugin not updated? @janosh
There was a problem hiding this comment.
yes, the spell checker fixes it. i don't think it makes sense to override the spell checker here since it is actually correct that this is not a word
There was a problem hiding this comment.
Maybe you want to consider add it to the workplace settings? In VScode it's just right click-spelling-Add words to workplace settings. I think it make sense to consider some package-specific terminologies like this, just like CONTCAR itself is not a word, but I still add it to VASP-related project workplaces to avoid being warned anyway.
| @@ -177,8 +178,7 @@ | |||
| fig = ptable_scatters( | |||
| data_dict, colormap="coolwarm", cbar_title="Periodic Table Scatter Plots" | |||
| ) | |||
|
|
|||
| save_and_compress_svg(fig, "ptable-scatters") | |||
| # save_and_compress_svg(fig, "ptable-scatters") | |||
There was a problem hiding this comment.
Commented out by mistake I guess?
There was a problem hiding this comment.
I would remove the comment in a follow up ptable_scatter revision PR and I would see if I could accomplish those improvements as you suggested 😊.
I would see if I could do anything soon. Tomorrow would be the lunar new year ✨ so...happy Chinese new year and I would catch up soon. |
no worries, i'll go ahead and merge this now. we can always make improvements later again, thanks so much for this contribution! 👍 |
* add ptable_hists example * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * copy ptable_hists and clean up comment * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * use colorbar instead of color bar * add ptable_scatters * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * show y axis and ticks * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * update main readme to include ptable _scatters * remove space in colormap/colorbar * replace _ with - * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * set tick position to out * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * use explicit encoding and absolute path * add ptable_scatters to __init__ * tweak comments * add unit tests * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * allow data in np.array as suggested by @janosh * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * compress assets/ptable-(hists|scatters).svg * fix ptable-hists, ptable-scatters SVG urls * clean up outdated gitignore * use shorter version of GH raw URLs * breaking: remove return_axes from ptable_hists() and ptable_scatters() seems of little use in hindsight * refactor tests * bump pre-commit hooks --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Janosh Riebesell <[email protected]>
closes #119
TODOs:
ptable_histsptable_scatterptable_scatterDict[str, np.ndarray]as dataptable_scatter(test all possible input data types)