Skip to content

Phase Diagram Serialization Improvement #4414

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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

fadams-umd
Copy link

Summary

Addresses #3940 by modifying the serialization of pymatgen.analysis.phase_diagram.PhaseDiagram to (1) remove redundant copies of the list of all the entries and (2) serialize the subset qhull_entries as a list of indices from which the list of entries can be recreated upon deserialization.

@mkhorton
Copy link
Member

mkhorton commented Jun 2, 2025

Thanks @fadams-umd, this seems much more efficient.

For backwards compatibility, for those who have already serialized a phase diagram with the older method, could you add an appropriate from_dict() method that can handle both formats?

@fadams-umd
Copy link
Author

Certainly! I just added a check for a dictionary key only present in the old format, and adjusted deserialization accordingly.

@shyuep shyuep enabled auto-merge (squash) June 10, 2025 14:08
@mkhorton
Copy link
Member

We're getting a test failure on analysis.test_phase_diagram.TestPhaseDiagram.test_as_from_dict that's blocking the merge. It looks like the test output just needs to be updated to reflect the changes made in this PR.

auto-merge was automatically disabled June 19, 2025 16:07

Head branch was pushed to by a user without write access

@fadams-umd
Copy link
Author

I was inadvertently modifying the dictionary argument in the from_dict method, which was throwing off the tests. Creating a copy solved the issue.

@CompRhys
Copy link
Contributor

@fadams-umd thanks for working on this! I originally noted this issue when working with patched phase diagrams which inflate this issue even more due to shared copies between patches. The only reasonable way to store them currently is as pickles which is an awful way to persist long term data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants