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

Backwards compatibility of test data with pymatgen #206

Merged
merged 6 commits into from
Apr 2, 2024

Conversation

ml-evs
Copy link
Collaborator

@ml-evs ml-evs commented Mar 29, 2024

As described in #204, our test data is losing compatibility with newer versions, as we used pickle rather than to_dict to serialize it. In the longer term, we will probably have to regenerate this data or refactor our tests, but for now this PR hot patches the loaded pickles with the required attributes for latest pymatgen versions.

This (somewhat bizarrely) allows more matminer featurizers to run successfully, so I had to also allow new features to be added on top of the reference data. This PR also allows the matminer ignore_errors arg to be controlled at the level of a featurizer, which is useful for debugging.

@ml-evs ml-evs force-pushed the ml-evs/more-deps branch from c2ce382 to 65d7990 Compare March 29, 2024 11:49
@ml-evs ml-evs force-pushed the ml-evs/more-deps branch from 65d7990 to 75914bb Compare March 29, 2024 11:58
@ml-evs ml-evs marked this pull request as ready for review March 29, 2024 15:01
@ml-evs
Copy link
Collaborator Author

ml-evs commented Mar 29, 2024

Hi @ppdebreuck, I was able to get everything working with the latest pymatgen, so think this is ready to release now.

@ml-evs ml-evs requested a review from ppdebreuck March 29, 2024 15:02
Copy link
Owner

@ppdebreuck ppdebreuck left a comment

Choose a reason for hiding this comment

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

Looks great ! Thanks for this. I'll take charge of changing the way we save and load MODData's in the future.

@ppdebreuck ppdebreuck merged commit 024d720 into master Apr 2, 2024
4 checks passed
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.

2 participants