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

Small patch of optimade (workaround) #46

Closed
wants to merge 1 commit into from
Closed

Conversation

pedrobcst
Copy link
Owner

This is a patch workaround since as of optimade > 1.6.0, the pymatgen adapter from Optimade seems to be broken.

To avoid issues, we are currently doing Optimade CIF (to cif) adapater and parsing the CIF using pymatgen Structure (from_str, fmt='cif') object itself.

@ml-evs
I am not sure where I can report this, however since the optimade > 1.6.0, the Pymatgen adapter gives the following error:

File ~/anaconda3/envs/XerusMin/lib/python3.8/site-packages/optimade/adapters/structures/pymatgen.py:79, in _get_structure(optimade_structure)
     74 """Create pymatgen Structure from OPTIMADE structure"""
     76 attributes = optimade_structure.attributes
     78 return Structure(
---> 79     lattice=Lattice(attributes.lattice_vectors, attributes.dimension_types),
     80     species=_pymatgen_species(
     81         nsites=attributes.nsites,  # type: ignore[arg-type]
     82         species=attributes.species,
     83         species_at_sites=attributes.species_at_sites,  # type: ignore[arg-type]
     84     ),
     85     coords=attributes.cartesian_site_positions,
     86     coords_are_cartesian=True,
     87 )

TypeError: __init__() takes 2 positional arguments but 3 were given

When using:

pymatgen_structure = structure.convert("pymatgen")

Where:

 structure = Structure(entry)

And entry is coming from an optimade query

Partially patches the Structure from optimade as the "pymatgen" converter is not working in the newest version (optimade > 1.2.0). First convert using the Structure adaptater to CIF and transforms to pymatgen structure by doing from_str from pymatgen Structure object itself
@pedrobcst pedrobcst requested a review from ml-evs January 30, 2023 01:30
@ml-evs
Copy link
Collaborator

ml-evs commented Jan 30, 2023

This looks like an incompatibility between optimade-python-tools and pymatgen version (cannot test right now as I am on my phone, but I'm fairly certain).

optimade>=0.16 (or thereabouts) made the switch to pymatgen>=2022 so that our other deps could be upgraded. If you specify the optimade requirement as optimade[pymatgen] then you will always get the correct versions. I guess you might be using the older style pymatgen calls elsewhere in the code though?

Although I can't promise it would always work (I probably wouldn't test it in the CI), we could add a workaround in optimade-python-tools that checks which pymatgen version you are using and either passes or does not pass dimension_types to the lattice constructor (which is the cause of the bug here).

@pedrobcst
Copy link
Owner Author

pedrobcst commented Jan 30, 2023

I see! Maybe I did something wrong at my end. As you said its a pymatgen issue version. However, since last version of Xerus, I had moved all the codebase to the newest pymatgen version (especially since to use the API new on the old version, you have to go to the legacy API page to get a working key, its not intutive at all), but I might had tested locally wrong. I will check that again on clean install, and if no issues I will just delete this PR.

EDIT: Yup, just confirmed, bumping up pymatgen goes smooth. I will change requirements.txt and close this PR.

The test failures here is coming from some new Conda issues (seems that Conda cannot create the enviroment based on the current files), that I will possibly think about it later..

@pedrobcst pedrobcst closed this Jan 30, 2023
@ml-evs
Copy link
Collaborator

ml-evs commented Jan 30, 2023

Okay, glad it's not something broken at our end 😁

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.

None yet

2 participants