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

Systematically improve fix pimd #3625

Closed
wants to merge 103 commits into from
Closed

Systematically improve fix pimd #3625

wants to merge 103 commits into from

Conversation

Yi-FanLi
Copy link
Collaborator

Summary

This is a draft pull request to systematically improve fix pimd.
It is intended to add the following features:

  1. stochastic thermostat
  2. isotropic / anisotropic barostat (and triclinic in the future)
  3. multi-processing for each bead
  4. well-optimized inter-replica communication and synchronization scheme to guarantee speed
  5. a set of post-processing code to calculate the quantum estimators

Note that this is still a draft version. I use the current version to run productive PIMD tasks for water/ice myself. It might not be compatible with the previous version which uses Nose-Hoover chain thermostat.
Also, I am still working on profiling and optimizing this code. There might be some synchronization issues. I am trying to accelerate it.
This code may need some modifications before being merged.

Related Issue(s)

Author(s)

Yifan Li
Graduate student at Princeton University
yifanl@princeton.edu

Licensing

By submitting this pull request, I agree, that my contribution will be included in LAMMPS and redistributed under either the GNU General Public License version 2 (GPL v2) or the GNU Lesser General Public License version 2.1 (LGPL v2.1).

Backward Compatibility

The Nose-Hoover chain thermostat is disabled temporarily due to the complexity of implementation in PIMD and remains for further work. CMD is also temporarily disabled for the same reason.

Implementation Notes

The PIMD integrators with the Langevin thermostat and BZP barostat are implemented mostly following the approach in i-PI. The correctness is tested in both Lennard-Jones potential and liquid water / ice systems.

Post Submission Checklist

  • The feature or features in this pull request is complete
  • Licensing information is complete
  • Corresponding author information is complete
  • The source code follows the LAMMPS formatting guidelines
  • Suitable new documentation files and/or updates to the existing docs are included
  • The added/updated documentation is integrated and tested with the documentation build system
  • The feature has been verified to work with the conventional build system
  • The feature has been verified to work with the CMake based build system
  • Suitable tests have been added to the unittest tree.
  • A package specific README file has been included or updated
  • One or more example input decks are included

Further Information, Files, and Links

Yi-FanLi and others added 30 commits February 21, 2021 01:08
…lf the inverse of the normal mode frequency. Could not run correctly.
Changed the unmap function in domain.h and domain.cpp. Also removed the MPI_Barriers in fix_pimd4.cpp.
…, by changing the position of domain->unmap to post_force.
@akohlmey
Copy link
Member

akohlmey commented Feb 2, 2023

@Yi-FanLi I've had a cursory look over your submission. This looks almost like a complete re-implementation of the PIMD functionality. So my recommendation at this point would be to give the class and command a new name, and have it added as an additional fix style instead of extending/replacing the existing fix. Since you are using a langevin thermostat, it could be called fix pimd/langevin (and the class correspondingly FixPIMDLangevin, and the files fix_pimd_langevin.h,.cpp). That also lifts the burden of having to restore the Nose-Hoover thermostat unless you want to do it. The documentation for both styles could be on the same doc page with some paragraphs explaining the differences. We would then rename the original fix pimd to fix pimd/nvt (with backward compatibility to support old inputs that use fix pimd).

@athomps do you agree with this?

@sjplimp
Copy link
Contributor

sjplimp commented Feb 2, 2023

@akohlmey @athomps I think we should also get Yuxing Peng to take a look - he is the author of the existing fix pimd. I have his current email - will contact him.

@Yi-FanLi
Copy link
Collaborator Author

@akohlmey @sjplimp Thanks for your kind feedback. I am also thinking about rename the fix style. @sjplimp Did you hear back from Yuxing Peng? Does he have any comments on this PR?

Thanks,
Yifan

@sjplimp
Copy link
Contributor

sjplimp commented Feb 17, 2023

Yuxing said he could take a look, but hasn't had a chance yet. I think it is fine to go ahead and rework this to be a separate
(new version) of his original fix pimd - he can look at it later in the process

@akohlmey
Copy link
Member

@Yi-FanLi We need to have a discussion on how you move forward with this work.

@sjplimp and I have discussed it and agreed to rename the existing fix pimd to fix pimd/nvt. Since you are using a stochastic thermostat, this would then naturally become fix pimd/langevin or similar. This change also relieves you from the burden of maintaining backward compatibility with the original PIMD implementation and thus can set up keywords and options differently (it is still recommended to make this similar to other LAMMPS commands, though).

I have just implemented the rename of the existing fix in pull request #3658.

My suggestion for you is the following:

  • after PR Collected small changes and bug fixes #3658 is merged to the "develop" branch, you create a new feature branch based on develop
  • you make copies of your fix_pimd.cpp and fix_pimd.h files outside of LAMMPS
  • you abandon and close this pull request and delete its branch
  • on the new feature branch, you add the preserved files as fix_pimd_langevin.cpp and fix_pimd_langevin.h and also change the internal class names accordingly and the style name to pimd/langevin
  • you start adding documentation for your PIMD variant to doc/src/fix_pimd.rst. Since the functionality is similar, there is a benefit to document both fixes together. This saves a lot of redundancy and you can specifically also discuss the differences between the two fixes so that users can make an informed choice. I've already tried to prepare the text in the file in anticipation of it, so it will be less work to share the sections of the documentation where both are the same.

I have some more comments on your implementation and its programming style, but that is better addressed after we have the "big picture" setup sorted out. If you want to discuss in more details, I can either send you an invite to the LAMMPS developer slack or offer you an in-person meeting via zoom. Of course we can also continue discussing here, if you prefer. Please send an email with your contact info to my personal email in case you want to discuss via slack or zoom. I expect that if we sort out some of the basic parameters as early as possible and in direct conversation, it will save you and us time and effort.

We are looking forward to have more options in LAMMPS for users that want to do PIMD simulations.

@Yi-FanLi
Copy link
Collaborator Author

Hi @akohlmey , thanks for your kind reply. I am very happy that you gave me so many useful suggestions. I'll rename my fix style and create a new PR as long as #3658 is merged.
I am very excited to have a discussion with you. Please do send me the slack link. Moreover, since I live at Princeton, which is only one hour's drive from Temple University, I am also very happy to meet you in person (I think it can be more efficient than a zoom meeting). Will you be more comfortable to meet virtually or in person?

@Yi-FanLi Yi-FanLi mentioned this pull request Feb 24, 2023
19 tasks
@Yi-FanLi Yi-FanLi closed this Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants