Skip to content

Remove text background and fix z-order in structure_viz #139

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
May 10, 2024
Merged

Remove text background and fix z-order in structure_viz #139

merged 17 commits into from
May 10, 2024

Conversation

DanielYang59
Copy link
Collaborator

@DanielYang59 DanielYang59 commented May 8, 2024 •
edited
Loading

Summary

  • Remove text background
  • Fix z-order such that objects at the foreground would properly occlude those at the background
  • BREAKING: remove site_labels_bbox argument
  • Replace deprecated MPRester with mp_api API in _generate_assets.py

Before:

image

After:

struct-2d-mp-19017-Li4Mn0 8Fe1 6P4C1 6O16-disordered

@DanielYang59 DanielYang59 marked this pull request as ready for review May 8, 2024 04:38
@DanielYang59
Copy link
Collaborator Author

DanielYang59 commented May 8, 2024 •
edited
Loading

@janosh Please review this at your convenience. Thanks!

I added mp_api as an optional dependency (to replace the deprecated API) to data-src = ["matminer", "mp_api",] in pyproject.toml but I'm not sure if it's the right place.

Noticed a weird behavior

The oxidation states like 3+ in matbench-phonons-structures-2d.svg disappeared in the first run, but then showed up randomly later without any change in the code.

@DanielYang59 DanielYang59 marked this pull request as draft May 8, 2024 10:02
@DanielYang59 DanielYang59 marked this pull request as ready for review May 8, 2024 10:17
@janosh
Copy link
Owner

janosh commented May 9, 2024

nice work! 👍 definite improvement

Perhaps we should revise the plotter such that the symbol of elements at the back would be blocked by elements in the front?

maybe we can make it an option whether element symbols should be occluded, e.g. occlude_labels: bool = true and include it in this PR? zorder might help here

@janosh janosh added fix Bug fix PRs structure Structure viz related labels May 9, 2024
@DanielYang59
Copy link
Collaborator Author

Wonderful. Z order is exactly what we need. And we need to define the zorder (perhaps by distance to the viewpoint). I would do this today. Thanks for the motivation.

@DanielYang59 DanielYang59 marked this pull request as draft May 10, 2024 03:23
@DanielYang59 DanielYang59 marked this pull request as ready for review May 10, 2024 03:48
@DanielYang59
Copy link
Collaborator Author

DanielYang59 commented May 10, 2024 •
edited
Loading

All done. Please review @janosh. Thanks!

I decide to make this the default (and only) behavior, because I assume there is no good reason for objects in the foreground not to occlude those at the background.

Update: Introduced a breaking change- remove site_labels_bbox arg because I think it's not quite necessary to include a bbox, but want to hear about your comments.

@DanielYang59 DanielYang59 changed the title Make text background in structures transparent Make text background in structures transparent and fix z-order May 10, 2024
@DanielYang59 DanielYang59 changed the title Make text background in structures transparent and fix z-order Remove text background and fix z-order in structure_viz May 10, 2024
janosh
janosh reviewed May 10, 2024
View reviewed changes
assets/matbench-phonons-structures-2d.svg Outdated
Copy link
Owner

Choose a reason for hiding this comment

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

i think this file needs regenerating. the oxidation states are gone

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry I didn't notice this, yes as I mentioned here #139 (comment), the fetching behavior of oxidation states still inconsistent for some reason.

@janosh janosh force-pushed the transparent-background branch from 981297e to 8a4a32c Compare May 10, 2024 12:41
janosh
janosh approved these changes May 10, 2024
View reviewed changes
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.

thanks @DanielYang59! 👍

@janosh janosh enabled auto-merge (squash) May 10, 2024 12:42
@janosh janosh merged commit 39fd71e into janosh:main May 10, 2024
8 checks passed
@DanielYang59 DanielYang59 deleted the transparent-background branch May 10, 2024 12:45
janosh added a commit that referenced this pull request Mar 28, 2025
@DanielYang59 @janosh
Remove text background and fix z-order in structure_viz (#139)
762a2d0 
* make background transparent

* update MP API entrance

* update structure by mp_id

* update matbench structure

* add mp_api to data-src dep

* update matbench fig

* tweak comments

* add zorder

* fix zorder for cell and make occlude default

* update figures

* fix occlusion order

* update figures

* drop bbox completely

* breaking: remove `site_labels_bbox`

* remove `site_labels_bbox` from unit test

* move ExperimentalWarning to pymatviz/utils.py

* restore oxi states in matbench-phonons-structures-2d.svg

---------

Co-authored-by: Janosh Riebesell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janosh janosh janosh approved these changes

Assignees
No one assigned
Labels
fix Bug fix PRs structure Structure viz related
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@DanielYang59 @janosh