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

Add OPTIMADE adapter #3876

Merged
merged 4 commits into from
Aug 7, 2024
Merged

Conversation

ml-evs
Copy link
Contributor

@ml-evs ml-evs commented Jun 11, 2024

Summary

This PR adds a converter between the pymatgen.core.Structure object and an OPTIMADE structure resource, migrated primarily out of https://github.com/Materials-Consortia/optimade-python-tools/blob/master/optimade/adapters/structures/pymatgen.py with the aim to remove it from our library.

Todos

  • Finish off tests
  • See if it can be used internally in the pymatgen.ext.optimade client (future work)

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)

@shyuep
Copy link
Member

shyuep commented Jun 11, 2024

I would put this in pymatgen.ext rather than pymatgen.io. The ext are external interfaces to things like API resources. E.g., matproj is in pymatgen.ext. Thanks.

@mkhorton
Copy link
Member

Given this is purely for conversion to/from the OPTIMADE schema, io is appropriate (but yes, any API-calling code lives in ext).

Thanks for the PR @ml-evs !

@ml-evs ml-evs marked this pull request as ready for review July 24, 2024 17:07
@ml-evs
Copy link
Contributor Author

ml-evs commented Jul 24, 2024

Apologies, I made this during the OPTIMADE workshop and your responses got lost in the notifications -- happy for this to go wherever but agree with Matt that io seems more appropriate to me (given that more tools are adding "offline" OPTIMADE support).

I'll get around to adding a couple more tests to this, but I think the code itself is ready for review.

@ml-evs
Copy link
Contributor Author

ml-evs commented Jul 24, 2024

Ok, slightly concerned that rebasing this PR has lead to 395 files being changed...

@janosh
Copy link
Member

janosh commented Jul 24, 2024

Ok, slightly concerned that rebasing this PR has lead to 395 files being changed...

that's from the recent 9100860 which didn't ensure pymatgen is recognized as a first-party package

@ml-evs ml-evs force-pushed the ml-evs/optimade-adapter branch 6 times, most recently from 832a23a to e670eac Compare July 28, 2024 09:52
@ml-evs
Copy link
Contributor Author

ml-evs commented Jul 28, 2024

Ok, rebased this more intelligently this time, have beefed up the test a little and think its ready to go.

@janosh janosh mentioned this pull request Aug 2, 2024
@janosh janosh added enhancement A new feature or improvement to an existing one ext pymatgen ext package for API data fetching labels Aug 2, 2024
@janosh janosh force-pushed the master branch 2 times, most recently from e3fbc67 to 41e6d99 Compare August 3, 2024 19:01
@janosh janosh enabled auto-merge (squash) August 7, 2024 17:32
@janosh janosh merged commit fce45f6 into materialsproject:master Aug 7, 2024
33 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 ext pymatgen ext package for API data fetching
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants