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

Lammps Support #348

Open
wants to merge 56 commits into
base: master
Choose a base branch
from
Open

Lammps Support #348

wants to merge 56 commits into from

Conversation

jaclark5
Copy link
Contributor

@jaclark5 jaclark5 commented Mar 13, 2024

This is a Draft I am actively using this and expect it's ready to go, but still needs tests.

Resolves: Issue #349

This module will allow for alchemlyb to be used with LAMMPS outputs. An independent package for easily generating the LAMMPS input files will shortly follow, although the functions have been designed to be flexible.

In addition to the extract_dHdl and extract_u_nk functions, additional functions, extract_dHdl_from_u_n and extract_u_nk_from_u_n have been added to allow these data frames to be generated from potential energy information that is separable from the target variable (e.g., epsilon in the LJ).

@jaclark5 jaclark5 marked this pull request as draft March 13, 2024 14:09
@xiki-tempula
Copy link
Collaborator

xiki-tempula commented Mar 14, 2024

This is really good!
For going forward, might be good to add some dataset to the https://github.com/alchemistry/alchemtest and we can use those file for testing.

@orbeckst orbeckst added the LAMMPS MD engine label Mar 30, 2024
@xiki-tempula xiki-tempula mentioned this pull request Mar 31, 2024
@orbeckst orbeckst mentioned this pull request May 27, 2024
@xiki-tempula
Copy link
Collaborator

Also would be good to add the docs with regard to this as well.

@jaclark5
Copy link
Contributor Author

jaclark5 commented Jul 10, 2024

@xiki-tempula

  • I did update the rst files where appropriate in the docs, did you have something more in mind?
  • I am also working on an addition to alchemtests, but since this PR is actively used in my research I'm also doing some stress testing on the side before I'll open this up for review. I think I'm just about happy with it.
  • There are some changes to the convergence modules that I added to this branch for convenience with my workflow, but I'll be moving those changes to another branch for a separate PR.

Copy link

codecov bot commented Jul 10, 2024

Codecov Report

Attention: Patch coverage is 0% with 382 lines in your changes missing coverage. Please review.

Project coverage is 82.82%. Comparing base (190308e) to head (337aac5).

Files with missing lines Patch % Lines
src/alchemlyb/parsing/lammps.py 0.00% 382 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (190308e) and HEAD (337aac5). Click for more details.

HEAD has 4 uploads less than BASE
Flag BASE (190308e) HEAD (337aac5)
8 4
Additional details and impacted files
@@             Coverage Diff             @@
##           master     #348       +/-   ##
===========================================
- Coverage   98.78%   82.82%   -15.97%     
===========================================
  Files          28       29        +1     
  Lines        1982     2364      +382     
  Branches      354      462      +108     
===========================================
  Hits         1958     1958               
- Misses          2      384      +382     
  Partials       22       22               

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

Copy link
Collaborator

@xiki-tempula xiki-tempula left a comment

Choose a reason for hiding this comment

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

There is really a lot of stuff in this PR and makes it kind of hard to review. I have given some suggestion and will do another review when the tests are there.

src/alchemlyb/convergence/convergence.py Outdated Show resolved Hide resolved
src/alchemlyb/convergence/convergence.py Outdated Show resolved Hide resolved
src/alchemlyb/convergence/convergence.py Outdated Show resolved Hide resolved
src/alchemlyb/estimators/bar_.py Outdated Show resolved Hide resolved
src/alchemlyb/parsing/lammps.py Outdated Show resolved Hide resolved
src/alchemlyb/parsing/lammps.py Show resolved Hide resolved
src/alchemlyb/parsing/lammps.py Outdated Show resolved Hide resolved
src/alchemlyb/parsing/lammps.py Outdated Show resolved Hide resolved
src/alchemlyb/parsing/lammps.py Outdated Show resolved Hide resolved
src/alchemlyb/parsing/lammps.py Outdated Show resolved Hide resolved
@jaclark5 jaclark5 marked this pull request as ready for review September 5, 2024 14:06
jaclark5 and others added 18 commits September 19, 2024 11:52
* Add moving_average function for visualization and convergence testing

* Update versionadded

* Run Black

* Bug fix bar_.py states

* Update Changelog

* Update the docs

* Add tests

* Formatting to align with Black

* Update tests

* Refactor moving_average to align with forward_backward_convergence function

* Update tests

* Update test_convergence and lambda tests in convergence.moving_average

* Adjust convergence.py and tests for codecoverage

* black

* Update moving_average to block_average for more accurate descriptive name

* Address reviewer comments

* Update test to align with changed handling of dfs of different length in block_average

* Remove incorrect popagation of error in BAR

* Add tests and error catch for ill constructed BAR input, u_nk

* black

* Updated version comments

---------

Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>
- generated with https://bit.ly/cffinit
- authors and order from AUTHORS
- affiliations and ORCID from JOSS paper
- emails from paper or individuals

Co-authored-by: David L. Dotson <dotsdl@gmail.com>
- inferred DOI from proof
- assume final acceptance in 2024...
- added David Mobley @davidlmobley (original conception and design of alchemlyb)
- added Michael Shirts @mrshirts (original conception and design of alchemlyb)
- CHANGES: bump to 2.3.3
- add reminder to AUTHORS to also update CITATION.cff
Added all authors that were mentioned in the Acknowledgements of the JOSS
paper but were not in AUTHORS

- Wei-Tse Hsu @wehs7661 (2020) for code clean-up in a732380
- Jan Janssen  @jan-janssen (2022) for creating the conda-forge package
- Shujie Fan @VOD555 (2022) for initial code for fractional equilibration time
- Helmut Carter @helmutcarter (2024) for doc fix in alchemistry#356
AUTHORS entry had been forgotten in PR alchemistry#381
* fix bug introduced in PR alchemistry#381: there was a change to creating the delta_f_ matrix,
  which resulted in the columns and indices being tuples that were in the wrong order 
  for single lambda computations.
* ensure that columns are in the correct order by explicitly sorting
* add a test for the delta_f_ columns
- fix alchemistry#385
- use pyproject.toml instead of setup.py (note: may need to change README for PyPi to exclude
  banners)
- remove versioneer and use versioningit (use alchemlyb.__version__ directly where
  the version is needed, e.g., for sphinx docs)
- updated CHANGES for 2.4.0
- gitignore _version
- ignore commits for blame that contain black-reformatting
  (in particular we forgot to include alchemistry#280)
- close alchemistry#402
- use same title in CITATION.cff for paper and project
  (required for the JOSS paper publication alchemistry#71)
- added DOI for preferred citation in note because that is the
  only hint that shows up on zenodo)
replace `pandas.concat()` with `alchemlyb.concat()` in the tutorial (given that we explicitly tell users to use it)
@orbeckst
Copy link
Member

orbeckst commented Oct 7, 2024

There are plenty of conflicts that need to be resolved. I'd say, ideally rebase this branch against the current main branch.

I also opened alchemistry/alchemtest#95 to add data files to alchemlyb. I would actually prioritize tests over pretty much anything else as (1) the tests show how your code will be used (and that's important for review) and (2) without tests we cannot merge code.

@orbeckst orbeckst linked an issue Oct 7, 2024 that may be closed by this pull request
@jaclark5
Copy link
Contributor Author

I also opened alchemistry/alchemtest#95 to add data files to alchemlyb.

Thanks!

I would actually prioritize tests over pretty much anything else as (1) the tests show how your code will be used (and that's important for review) and (2) without tests we cannot merge code.

Yes, I've been putting together an example to add to alchemtest, but in the process I found a couple issues with the way I ran TI and BAR. At this point I'm 100% confident in the results and since alchemtest has a license associated with each simulation example I'm submitting my simulation data to our internal review process to associate the NIST license.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for FEP calculation in LAMMPS
3 participants