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

Split make_assets scripts by plot functions #247

Merged
merged 34 commits into from
Nov 19, 2024

Conversation

DanielYang59
Copy link
Collaborator

@DanielYang59 DanielYang59 commented Nov 18, 2024

@DanielYang59 DanielYang59 self-assigned this Nov 18, 2024
@DanielYang59 DanielYang59 added ux User experience dx Developer experience examples New or improved usage examples labels Nov 18, 2024
Copy link
Owner

@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 think this PR is mostly ready to go. just need to update the readme links to point to the asset scripts instead of the source code files

Comment on lines 17 to 22
plt.rc("font", size=14)
plt.rc("savefig", bbox="tight", dpi=200)
plt.rc("axes", titlesize=16, titleweight="bold")
plt.rc("figure", dpi=200, titlesize=20, titleweight="bold")
plt.rcParams["figure.constrained_layout.use"] = True

Copy link
Owner

Choose a reason for hiding this comment

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

would be good to centralize these settings instead of repeating them in 15 asset scripts. if you import a file which has these commands at the top level, the settings will take effect

Copy link
Collaborator Author

@DanielYang59 DanielYang59 Nov 19, 2024

Choose a reason for hiding this comment

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

Thanks for the help, it's super helpful :)

Yes of course, sorry I'm currently playing with Network UPS Tools trying to set it up for my toy server, I would get my hands on this later.

Also I might need to give the asset makers a complete check because for some reason the heatmap split plotter is more "sparse" than before

ptable-heatmap-splits-3

| ---------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------- |
| [mlff_phonons.ipynb](https://github.com/janosh/pymatviz/blob/main/examples/mlff_phonons.ipynb) | [![Open in Google Colab][Open in Google Colab]](https://colab.research.google.com/github/janosh/pymatviz/blob/main/examples/mlff_phonons.ipynb) | [Launch Codespace][codespace url] |
| [matbench_dielectric_eda.ipynb](https://github.com/janosh/pymatviz/blob/main/examples/matbench_dielectric_eda.ipynb) | [![Open in Google Colab][Open in Google Colab]](https://colab.research.google.com/github/janosh/pymatviz/blob/main/examples/matbench_dielectric_eda.ipynb) | [Launch Codespace][codespace url] |
| [mp_bimodal_e_form.ipynb](https://github.com/janosh/pymatviz/blob/main/examples/mp_bimodal_e_form.ipynb) | [![Open in Google Colab][Open in Google Colab]](https://colab.research.google.com/github/janosh/pymatviz/blob/main/examples/mp_bimodal_e_form.ipynb) | [Launch Codespace][codespace url] |
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

pre-commit seems to add a duplicate for section hyperlink header for some reason

Copy link
Owner

Choose a reason for hiding this comment

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

that's the table header. every markdown needs to have one unfortunately. it can be empty but it needs to be there else it won't render as a table

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for letting me know, admittedly this un-rendered markdown is painful to look at.

Weird these headers were not added before, but this See the [/api][/api] page. doesn't seem like a table header?

readme.md Show resolved Hide resolved
DanielYang59 and others added 4 commits November 19, 2024 16:48
mv tests/test/test_data.py to tests/test_data.py
…g_matplotlib to apply_matplotlib_template

add unit test for apply_matplotlib_template()
remove unneeded plotly templates from matplotlib asset scripts
Copy link
Owner

@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 think this is ready to go 👍

@janosh janosh marked this pull request as ready for review November 19, 2024 12:21
@DanielYang59
Copy link
Collaborator Author

Thanks for doing all heavy lifting :) I'm okay with merging this first (after fix those minor import issues) and apparently some asset makers are broken, which I believe it's not related to change here, but still worth extra attention

…rups/powerups.py

remove unused pmv.set_plotly_template
@janosh janosh enabled auto-merge (squash) November 19, 2024 12:31
@janosh janosh merged commit 4380b05 into janosh:main Nov 19, 2024
50 checks passed
@DanielYang59 DanielYang59 deleted the split-asset-maker branch November 19, 2024 12:32
@DanielYang59
Copy link
Collaborator Author

DanielYang59 commented Nov 19, 2024

Oops, my bad, looks like a typo assets/scripts/uncertainty//qq_gaussian.py, sorry I thought I manually checked all links earlier but still missed one (for some reason the link still works even with an extra slash /):

SvelteKitError: Not found: /assets/scripts/uncertainty//qq_gaussian.py

I will fix it in #248 69f554b

@janosh janosh added docs Improvements or additions to documentation and removed dx Developer experience labels Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to documentation examples New or improved usage examples ux User experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split make_assets scripts by plot function
2 participants