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 to AtomsBase v0.5 #57

Merged
merged 2 commits into from
Nov 28, 2024
Merged

Conversation

tjjarvinen
Copy link
Contributor

The main part for this update is renaming bounding_box to cell_vectors.

The other part is update with ChemicalSpecies for isotopes and masses. This had a small change too.

Finally AtomsBaseTesting had to be updated to a version that supports ABv0.5. This caused some updates on the testing part, which in general would need a facelift at somepoint.

I have not thesed this outside of test set. But it should be ok for majority of the use cases.

Copy link

codecov bot commented Nov 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.33%. Comparing base (0c84d2e) to head (53c2135).
Report is 25 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #57      +/-   ##
==========================================
- Coverage   94.75%   91.33%   -3.42%     
==========================================
  Files           3        3              
  Lines         381      381              
==========================================
- Hits          361      348      -13     
- Misses         20       33      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -89,7 +95,6 @@ end
atoms = @test_logs((:warn, r"Unitful quantity massdata is not yet"),
(:warn, r"Writing quantities of type Symbol"),
(:warn, r"Unitful quantity md is not yet"),
(:warn, r"Mismatch between atomic numbers and atomic symbols"),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is because setting the atomic_symbol atprop now has no longer the effect of changing the atomic symbol as this is handled by the species. I'd suggest to still trigger this warning in the tests though by setting custom species and including a deuterium in there. This should be doable just by replacing atomic_symbol=[ ... ] by species=ChemicalSpecies.([ .. ]).

@mfherbst
Copy link
Contributor

Heyho, just wanted to mention that this PR is now the blocker for me to move more of our ecosystem to AB 0.5. Happy to help getting this merged as fast as possible.

@tjjarvinen
Copy link
Contributor Author

I am not sure what to do with Windows nightly. Looking for that might be the way to help. But I am not sure is anything to do with IO issues with Windows #55.

Codecov part is just adding more tests. Not sure is it that important anyway...

@cortner
Copy link
Member

cortner commented Nov 28, 2024

How many windows users do we have in this community? Maybe they can fix this? If not, then remove windows from tests or make it an allowed fail.

@mfherbst
Copy link
Contributor

Unless there are strong opinions, I'd not bother too much with Windows.

@cortner
Copy link
Member

cortner commented Nov 28, 2024

Unless there are strong opinions, I'd not bother too much with Windows.

I have a strong opinion to not bother too much with windows.

@jameskermode
Copy link
Member

Thanks for the PR. I’ll go ahead and merge. Windows tests are actually working on Julia 1.10 in any case.

@jameskermode jameskermode merged commit 5df541d into libAtoms:master Nov 28, 2024
8 of 9 checks passed
@jameskermode
Copy link
Member

0.2.2 should appear shortly

@tjjarvinen tjjarvinen deleted the ABv0.5_update branch November 29, 2024 11:32
@mfherbst mfherbst mentioned this pull request Nov 29, 2024
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.

4 participants