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

Make structure adapters infer species from species_at_sites when missing #1103

Merged
merged 1 commit into from
Mar 23, 2022

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Mar 20, 2022

I keep running into this issue where implementations do not return species by default, because their species dictionaries are extremely simple (i.e., single chemical symbol per species, no disorder, and the name follows the element symbol).

This PR allows the ASE, AiiDA and pymatgen structure adapters to infer this simple structure from just species_at_sites when species is missing/None.

@ml-evs
Copy link
Member Author

ml-evs commented Mar 20, 2022

I need this for a demo of the materials resource registry coming up soon, so will merge and release in a few days if there are no complaints.

@codecov
Copy link

codecov bot commented Mar 20, 2022

Codecov Report

Merging #1103 (71136d7) into master (8dcde4d) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1103      +/-   ##
==========================================
+ Coverage   92.30%   92.33%   +0.02%     
==========================================
  Files          68       68              
  Lines        3834     3849      +15     
==========================================
+ Hits         3539     3554      +15     
  Misses        295      295              
Flag Coverage Δ
project 92.33% <100.00%> (+0.02%) ⬆️
validator 92.33% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
optimade/adapters/structures/aiida.py 100.00% <100.00%> (ø)
optimade/adapters/structures/ase.py 97.29% <100.00%> (+0.32%) ⬆️
optimade/adapters/structures/pymatgen.py 100.00% <100.00%> (ø)
optimade/adapters/structures/utils.py 81.30% <100.00%> (+0.46%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8dcde4d...71136d7. Read the comment docs.

Copy link
Contributor

@JPBergsma JPBergsma left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@ml-evs
Copy link
Member Author

ml-evs commented Mar 21, 2022

Thanks @JPBergsma! I think @CasperWA is the main user of these adapters within other projects, so I'll just wait for the OK from him too.

@ml-evs
Copy link
Member Author

ml-evs commented Mar 23, 2022

I've had a look at the code that uses these adapters and I'm fairly certain that these changes are fine; will merge and release.

@ml-evs ml-evs merged commit 0660556 into master Mar 23, 2022
@ml-evs ml-evs deleted the ml-evs/flexi-adapters branch March 23, 2022 17:40
@ml-evs ml-evs added enhancement New feature or request adapters Issues pertaining to adapters (converters) labels Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adapters Issues pertaining to adapters (converters) enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants