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

Add create_atoms function to LAMMPS.jl #60

Merged
merged 14 commits into from
Aug 14, 2024
Merged

Add create_atoms function to LAMMPS.jl #60

merged 14 commits into from
Aug 14, 2024

Conversation

jmeziere
Copy link
Contributor

@jmeziere jmeziere commented Aug 4, 2024

It is useful (at least for me) to be able to load atoms directly from Julia to LAMMPS. This pull request provides a wrapper around lammps_create_atoms to easily do this, similar to the create_atoms function in the python interface to LAMMPS.

Copy link
Collaborator

@Joroks Joroks 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 the PR, this looks great!

One thing I'm not quite sure about, is the handling of datatypes not supported directly by lammps. Up until now we've just restricted the wrapper functions to be called with the correct datatype.

I understand that it would be convenient for the user if we'd just handle the type conversion where needed but I also don't think that it would be too much to ask to have the user do this themselves.

If we ultimately decide on handling the type conversion on our end, we'd have to make this consistent across our other methods as well. I also think that we shouldn't issue warnings in case of a type mismatch but rather just document this behavior in our docstrings.

@vchuravy what's your opinion on this?

src/LAMMPS.jl Outdated Show resolved Hide resolved
src/LAMMPS.jl Outdated Show resolved Hide resolved
@vchuravy
Copy link
Member

vchuravy commented Aug 5, 2024

Yeah I would rather check the DType and then error

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.33%. Comparing base (913e7c3) to head (5d704de).
Report is 5 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##             main      #60       +/-   ##
===========================================
+ Coverage   54.56%   72.33%   +17.76%     
===========================================
  Files           2        2               
  Lines         449      459       +10     
===========================================
+ Hits          245      332       +87     
+ Misses        204      127       -77     

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

Copy link
Collaborator

@Joroks Joroks left a comment

Choose a reason for hiding this comment

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

@vchuravy could you take a quick look at this before I merge it?

src/LAMMPS.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
Co-authored-by: Valentin Churavy <[email protected]>
test/runtests.jl Outdated Show resolved Hide resolved
src/LAMMPS.jl Outdated Show resolved Hide resolved
src/LAMMPS.jl Outdated Show resolved Hide resolved
src/LAMMPS.jl Outdated Show resolved Hide resolved
Copy link
Collaborator

@Joroks Joroks left a comment

Choose a reason for hiding this comment

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

Please be a bit more thorough when revising your PR.
Testing that your code is working by typing ] test in your REPL is allways a good idea too.

test/runtests.jl Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
src/LAMMPS.jl Show resolved Hide resolved
Copy link
Collaborator

@Joroks Joroks left a comment

Choose a reason for hiding this comment

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

I think this should now be good to go.

src/LAMMPS.jl Show resolved Hide resolved
src/LAMMPS.jl Outdated Show resolved Hide resolved
@Joroks Joroks merged commit b60ac24 into cesmix-mit:main Aug 14, 2024
14 of 16 checks passed
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