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

Update species->mass field following specification change #631

Merged
merged 7 commits into from Feb 19, 2021

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Dec 17, 2020

Closes #630 following changes introduced in Materials-Consortia/OPTIMADE#344.

@ml-evs ml-evs added schema Concerns the schema models priority/high Issue or PR with a consensus of high priority models For issues related to the pydantic models directly OPTIMADE v1.0 A label for tagging issues/PRs that are required for compliance with v1.0 of the OPTIMADE spec labels Dec 17, 2020
@ml-evs ml-evs force-pushed the ml-evs/close_#630 branch 2 times, most recently from 5e75077 to b13de87 Compare December 17, 2020 22:46
@ml-evs ml-evs marked this pull request as ready for review December 17, 2020 22:54
@codecov
Copy link

codecov bot commented Dec 21, 2020

Codecov Report

Merging #631 (76b70c7) into master (2f4d721) will increase coverage by 0.00%.
The diff coverage is 97.05%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #631   +/-   ##
=======================================
  Coverage   93.18%   93.19%           
=======================================
  Files          61       62    +1     
  Lines        3362     3381   +19     
=======================================
+ Hits         3133     3151   +18     
- Misses        229      230    +1     
Flag Coverage Δ
project 93.19% <97.05%> (+<0.01%) ⬆️
validator 65.83% <14.70%> (-0.32%) ⬇️

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

Impacted Files Coverage Δ
optimade/adapters/structures/ase.py 96.96% <85.71%> (-3.04%) ⬇️
optimade/adapters/structures/aiida.py 100.00% <100.00%> (ø)
optimade/adapters/structures/cif.py 100.00% <100.00%> (ø)
optimade/adapters/structures/jarvis.py 100.00% <100.00%> (ø)
optimade/adapters/structures/proteindatabank.py 100.00% <100.00%> (ø)
optimade/adapters/structures/pymatgen.py 100.00% <100.00%> (ø)
optimade/adapters/warnings.py 100.00% <100.00%> (ø)
optimade/models/structures.py 96.38% <100.00%> (+0.02%) ⬆️

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 2f4d721...76b70c7. Read the comment docs.

@CasperWA
Copy link
Member

@ml-evs To sum up:

  • I've reverted the change I did to the ASE adapter, since it was correct, as we're not taking species with multiple chemical_symbols into account.
  • I've added another "special species" for testing the adapters, specifically only using it for the AiiDA adapter for now.
  • The calculation of an AiiDA Kind mass is done by multiplying each mass with the concentration and summing it all up.
    Thinking about this, I'm not completely sure this is correct, but I might be messing it all up in my own head thinking about chemistry, whereas this is not really chemistry... Hmm. Let's perhaps discuss on slack?

@CasperWA
Copy link
Member

Further updates:

  • Created specific adapter Warnings and used them for the fallback AiiDA mass setting warning, as well as for the general warning when a Python package cannot be found in the current environment when trying to use the adapter for said package.
  • Updated pytest.ini to ignore a sleugh of Warnings.

@CasperWA
Copy link
Member

Maybe the adapter Warnings should not subclass the server base Warning? But then the doc string for said base Warning should definitely be updated.

@ml-evs
Copy link
Member Author

ml-evs commented Feb 17, 2021

Is there anything still holding this up @CasperWA? I'd completely forgotten about it, but we should have really gotten this into the last openapi schema update ready for spec v1.0.1

@CasperWA
Copy link
Member

Is there anything still holding this up @CasperWA? I'd completely forgotten about it, but we should have really gotten this into the last openapi schema update ready for spec v1.0.1

I need to dig into it again before I can answer that question. But from a quick look it seems it's all about just testing your suggestion and that it catches the correct edge cases?

@ml-evs
Copy link
Member Author

ml-evs commented Feb 17, 2021

Is there anything still holding this up @CasperWA? I'd completely forgotten about it, but we should have really gotten this into the last openapi schema update ready for spec v1.0.1

I need to dig into it again before I can answer that question. But from a quick look it seems it's all about just testing your suggestion and that it catches the correct edge cases?

I would be strongly in favour of splitting this PR into the model changes and the adapter changes, so that we can get the model changes in before the meeting today...

ml-evs and others added 7 commits February 17, 2021 22:22
Fix masses creation for AiiDA StructureData to take concentration into
account.
Subclassing `optimade.server.warnings.OptimadeWarning`.

Use new warnings in all adapters for reporting if adapter package is not
installed.
Use new warning for AiiDA if mass is set to the fallback value of 1.0.

Update tests accordingly.

Update pytest.ini to avoid printing a lot of the warnings messages.
@CasperWA
Copy link
Member

I would consider this all good to go.

@ml-evs ml-evs merged commit 04659d4 into master Feb 19, 2021
@ml-evs ml-evs deleted the ml-evs/close_#630 branch February 19, 2021 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
models For issues related to the pydantic models directly OPTIMADE v1.0 A label for tagging issues/PRs that are required for compliance with v1.0 of the OPTIMADE spec priority/high Issue or PR with a consensus of high priority schema Concerns the schema models
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update species.mass model
2 participants