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

AtomsBase testing #131

Merged
merged 12 commits into from
Jul 13, 2023
Merged

AtomsBase testing #131

merged 12 commits into from
Jul 13, 2023

Conversation

ejmeitz
Copy link
Contributor

@ejmeitz ejmeitz commented May 29, 2023

This PR will add better support of AtomsBase types and implement AtomsBaseTesting into the CI.

From #128

@ejmeitz ejmeitz marked this pull request as draft May 29, 2023 23:47
@codecov
Copy link

codecov bot commented May 30, 2023

Codecov Report

Patch coverage: 75.00% and project coverage change: +0.55 🎉

Comparison is base (59807dd) 72.83% compared to head (b3867bf) 73.38%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #131      +/-   ##
==========================================
+ Coverage   72.83%   73.38%   +0.55%     
==========================================
  Files          34       34              
  Lines        4804     5024     +220     
==========================================
+ Hits         3499     3687     +188     
- Misses       1305     1337      +32     
Impacted Files Coverage Δ
src/Molly.jl 100.00% <ø> (ø)
src/types.jl 72.75% <75.00%> (+1.32%) ⬆️

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ejmeitz
Copy link
Contributor Author

ejmeitz commented Jun 3, 2023

I think this is pretty much done from code that needs to be added to Molly. I made some PRs in AtomsBase to add features that Molly requires and to make the testing framework more flexible.

@ejmeitz
Copy link
Contributor Author

ejmeitz commented Jun 23, 2023

Asssuming the tests pass this is probably good enough to cover what @mfherbst was asking for in #128

Based on how this PR ion AtomsBase turns out I might have to make modifications later. For now I think it is good enough but welcome suggestions/edits.

Edit: Need to fix issue with Atom being exported in AtomsBase and Molly & breaking the other tests.

@ejmeitz ejmeitz marked this pull request as ready for review June 24, 2023 17:18
Copy link
Member

@mfherbst mfherbst left a comment

Choose a reason for hiding this comment

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

Some comments from my end.

src/types.jl Outdated Show resolved Hide resolved
src/types.jl Outdated Show resolved Hide resolved
src/types.jl Outdated Show resolved Hide resolved
src/types.jl Outdated Show resolved Hide resolved
src/types.jl Show resolved Hide resolved
src/types.jl Show resolved Hide resolved
Copy link
Collaborator

@jgreener64 jgreener64 left a comment

Choose a reason for hiding this comment

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

Thanks for improving compatibility here. Some of the stuff with keys and fields seems a little strange, that may be the price of supporting AtomsBase though. The ability to convert from an AtomsBase system is useful.

test/atomsbase.jl Outdated Show resolved Hide resolved
src/types.jl Outdated Show resolved Hide resolved
src/types.jl Outdated Show resolved Hide resolved
src/types.jl Outdated Show resolved Hide resolved
src/types.jl Outdated Show resolved Hide resolved
src/types.jl Outdated Show resolved Hide resolved
src/types.jl Outdated Show resolved Hide resolved
@ejmeitz
Copy link
Contributor Author

ejmeitz commented Jul 12, 2023

@jgreener64 all good!

@jgreener64 jgreener64 merged commit b0aa17b into JuliaMolSim:master Jul 13, 2023
7 checks passed
@ejmeitz ejmeitz deleted the AtomsBaseTesting branch July 13, 2023 12:03
Comment on lines +425 to +433
system = make_test_system().system;
#Update values to be something that works with Molly
system = AbstractSystem(system;
boundary_conditions = [Periodic(), Periodic(), Periodic()],
bounding_box = [[1.54732, 0.0, 0.0],
[0.0, 1.4654985, 0.0],
[0.0, 0.0, 1.7928950]]u"Å");
molly_sys = System(system)
test_approx_eq(system, molly_sys; common_only = true)
Copy link
Member

@mfherbst mfherbst Jul 26, 2023

Choose a reason for hiding this comment

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

Sorry, only got round to look at this now. This is not the best way to do this as with common_only=true you make the tests less rigorous. You can directly overwrite things (or drop keys) from make_test_system and then one should be able to use test_approx_eq directly without using the common_only=true flag.

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

3 participants