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

Improve Keep Redundant Spaces algorithm for PatchedPhaseDiagram #3900

Merged
merged 5 commits into from
Jul 17, 2024

Conversation

CompRhys
Copy link
Contributor

@CompRhys CompRhys commented Jun 28, 2024

Summary

  • modularize the keep redundant spaces code, and fix the algorithm which previously didn't get rid of all the redundant spaces.

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)

@janosh janosh added enhancement A new feature or improvement to an existing one performance Some functionality is too slow or regressed labels Jun 28, 2024
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.

PatchedPhaseDiagrams pickled to disk with this change should result in smaller files as fewer sub phase diagrams need to be stored

@CompRhys just corrected me, all this is saving are some memory IDs which are on the order of a few bytes so no meaningful decrease in file size expected

@mkhorton
Copy link
Member

Thanks @CompRhys!

I think some comment strings need to be updated: for example, there is a claim that PatchedPhaseDiagram uses the same as_dict() as PhaseDiagram but this is not the case (computed_data, i.e. including the qhull entries, are not stored). Otherwise this looks great.

CompRhys and others added 3 commits July 17, 2024 10:25
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.

@CompRhys thanks for addressing the outdated doc strings!

@janosh janosh merged commit 0234182 into materialsproject:master Jul 17, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement to an existing one performance Some functionality is too slow or regressed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants