Harden PLUMED restart by danielhollas · Pull Request #53 · PHOTOX/ABIN

danielhollas · 2021-01-11T22:43:55Z

Previously, in order to restart the PLUMED simulations,
the 'RESTART' keyword needed to be added at the beginning
of the PLUMED input file, in addition to setting 'irest=1' in the ABIN input file.

In this diff, we pass the restart info from ABIN to PLUMED directly
so the additional keyword in the PLUMED input is not needed
(but it can still be there).

In addition, I added some basic unit tests for the PLUMED
integration and I improved the current PLUMED test:

to test the restart capability
print more output files from PLUMED
to check we're passing correct info from ABIN.
Specifically, we check masses and energies.

I also added a test for compilation without PLUMED.

Finally, I added the brand new 2.7 PLUMED version to GitHub CI.

Because of these changes, I needed to bump the minimum PLUMED API version.
Nevertheless, the 2.5 version still works. Since 2.5 is the last officially supported version,
I am not worried about compatibility with older versions.

Also, I find out that temperature was passed incorrectly into PLUMED! (it was set to zero). I commented out the call
so PLUMED should now die if temperature is not specified in the plumed input. I'll fix this in a future PR. Having
to specify temperature in two different places is clearly suboptimal.

codecov · 2021-01-11T22:45:17Z

Codecov Report

Merging #53 (03d95ca) into master (28e01f6) will increase coverage by 0.05%.
The diff coverage is 90.58%.

@@            Coverage Diff             @@
##           master      #53      +/-   ##
==========================================
+ Coverage   59.71%   59.77%   +0.05%     
==========================================
  Files          36       36              
  Lines        5575     5593      +18     
==========================================
+ Hits         3329     3343      +14     
- Misses       2246     2250       +4

Impacted Files	Coverage Δ
src/modules.F90	`67.20% <ø> (ø)`
src/plumed.F90	`89.33% <89.33%> (-7.34%)`	⬇️
src/init.F90	`60.43% <100.00%> (+0.06%)`	⬆️
src/utils.F90	`84.29% <100.00%> (+0.96%)`	⬆️

danielhollas · 2021-01-13T18:13:29Z

      fail-fast: false
      matrix:
-         plumed_v: [2.5.3, 2.6.2]
+         plumed_v: [2.5.3, 2.6.2, 2.7.0]


New version has just been released so I added it here. :-)

danielhollas · 2021-01-13T18:15:00Z


   if(iplumed.eq.1) then
-
-#ifdef USE_PLUMED


Looks much nicer and more readable without ifdefs everywhere. :-)

danielhollas · 2021-01-13T18:40:28Z

      end do
   end function LowerToUpper

+   subroutine not_compiled_with(feature, caller)


Just a simple helper function to make our errors uniform and avoid code duplication. Right now only used in PLUMED interface, but I'll use it for MPI and others in future PRs.

Ok, this is clever.

danielhollas · 2021-01-14T13:13:45Z


 .SUFFIXES: .F90
-
-.F90.o:


This rule was not actually being used (I think it was being overwritten internally by pFUnit.
So instead I added $(DFLAGS) to $(FFLAGS) above.

danielhollas · 2021-01-14T14:31:23Z

+      ! NOTE: 'readInputLine' cannot be called before 'init'
+      ! Only available for API_VERSION > 3
+      ! https://www.plumed.org/doc-v2.6/user-doc/html/_f_l_u_s_h.html
+      call plumed_f_gcmd(c_string('readInputLine'), 'FLUSH STRIDE='//c_string(chint))


This is a new thing, PLUMED by default flushes only once per 1000, so if ABIN died unexpectedly (e.g. electrical failure), the data would be lost.

Yes, I frequently used it because 1000 steps in ab initio world can take ages and meanwhile you cannot work on the data thats already there. I wonder what gets priority if you redefine flush stride in plumed.in. Maybe we could check whether chint (= nrest) > 1000 ... lets wait for the user experience.

Good question. I'll test it out.

It seems that PLUMED considers FLUSH as any other ACTION so if you provide it more than once, it will presumably flush files more than once (possibly with different frequency). So I think we're good here. By default, nrest=1 in ABIN so this feels as a good approach. Setting up nrest to something much higher doesn't make sense, and even then PLUMED will simply flush at its default pace according to the doc:

Notice that all files are flushed anyway every 10000 steps.

danielhollas · 2021-01-14T14:32:06Z

+      call plumed_f_gcmd(c_string('readInputLine'), 'FLUSH STRIDE='//c_string(chint))
+   end subroutine plumed_init
+
+   subroutine finalize_plumed()


Just trying to encapsulate any direct calls to PLUMED interface to this mod_plumed module.

danielhollas · 2021-01-14T14:49:10Z

+      real(DP) :: fxx(size(fx, 1)), fyy(size(fy, 1)), fzz(size(fz, 1))
+      ! Currently not implemented
+      !real(DP) :: pbox(3,3), pcharges(natom)
+      real(DP) :: plumvirial(3, 3)


I wanted to get rid of this, but PLUMED crushes if we don't provide the virial.
On the other hand, I was able to get rid of the charges, which we do not support,
and PLUMED now errors out (as it should) if a user requests an Action based on charges.

danielhollas · 2021-01-14T14:51:05Z

-   ! (https://aip.scitation.org/doi/abs/10.1063/1.4986915)   
-   iw = 1
-
-   do iat = 1, natom


We now set mass units in plumed_init so this conversion here is not needed anymore. I added a test for this in tests/PLUMED

danielhollas · 2021-01-14T14:57:49Z

+      real(DP), intent(inout) :: eclas
+
+      ! Just to get rid of compiler warnings :-(
+      fx = x; fy = y; fz = z; eclas = 0.0D0


Ideas on how to get rid of this are welcomed. :-)

danielhollas · 2021-01-14T15:06:04Z

+      call plumed_f_gcmd(c_string("setTimestep"), dt0)
+      call plumed_f_gcmd(c_string("setLogFile"), c_string(plumedoutfile))
+
+      if (irest == 1) then


Here's the new thing.

danielhollas · 2021-01-15T15:23:04Z

+      ! Also, this conversion only works if temp is in Kelvins,
+      ! but we use temp in atomic units in ABIN!
+      ! Verify kbt using https://www.plumed.org/doc-v2.5/user-doc/html/kt.html
+      !plumed_kbt = temp * 8.3144598D0 ! in kJ/mol


This was a bug, not sure when it was introduced or if this ever worked. This could potentially influenced old well-tempered metadynamics simulations, if the user forgot to specify TEMP in the PLUMED input. In that case, we passed zero temperature here. :-(

I think, thanks to the exponential in WTMetaD formula this would induce HILLS with constant or zero height? Anyway good catch.

Yeah, I think you'd have noticed if this has been an issue. But it will be good to have this fixed.

danielhollas · 2021-01-15T15:32:49Z

@suchanj this is ready for review. Happy to answer any questions. I ended up doing a general cleanup of plumed.F90 and I also applied fprettify to it so it's a bit harder to review, sorry about that. That are only a couple of meaningful changes though, I tried to point those out with comments.

I spent some time figuring out how to write unit test for a conditionally-compiled feature, so this PR will be a good reference for that. I have one remaining TODO in the new unit tests. I also would like to go through all the steps from the debugging PLUMED doc: https://www.plumed.org/doc-v2.7/developer-doc/html/_how_to_plumed_your_m_d.html#debugging-patch
These are the ones I already checked:

I maybe write some more tests in the next PR. I also need to fix the passing of temperature from ABIN to PLUMED, see comments in the code. I'll write a test for well-tempered metadynamics for that.

danielhollas · 2021-01-15T16:32:17Z

I verified that the code coverage looks good. The relative drop in coverage in plumed.F90 is just because of an auxiliary function that is never called when ABIN is compiled without PLUMED (and because the total number of code lines has dropped).
In reality we're now covering more because we didn't test restarting with PLUMED at all.

suchanj · 2021-01-18T09:48:45Z

+   real(DP), parameter :: AUTOJ = 4.3597447222071D-18 ! hartree -> J
   real(DP), parameter :: AMU=1822.888484264545d0 ! atomic mass unit 
-   real(DP), parameter :: ANG=1.889726132873d0     ! nagstroms to bohrs
+   real(DP), parameter :: ANG = 1.889726132873D0    ! Agstroms To Bohrs


We both droped "n" in the word aNgstrom

Good catch. :-) Thanks for the review!

Previously, in order to restart the PLUMED simulations, the 'RESTART' keyword needed to be added at the beginning of the PLUMED input file, in addition to setting 'irest=1' in the ABIN input file. In this diff, we pass the restart info from ABIN to PLUMED directly so the additional keyword in the PLUMED input is not needed (but it can still be there). In addition, I added some basic unit tests for the PLUMED integration and I improved the current PLUMED test: 1. to test the restart capability 2. print more output files from PLUMED to check we're passing correct info from ABIN. Specifically, we check masses and energies. I also added a test for compilation without PLUMED. Finally, I added the brand new 2.7 PLUMED version to GitHub CI. Because of these changes, I needed to bump up the minimum PLUMED API version. Nevertheless, the 2.5 version still works, and this is the last officially supported version so I am not worried about compatibility with older versions. Also, I find out that temperature was passed incorrectly into PLUMED! (it was set to zero). I commented out the call so PLUMED should now die if temperature is not specified in the plumed input. I'll fix this in a future PR. Having to specify temperature in two different places is suboptimal.

danielhollas · 2021-01-19T17:28:47Z

+      integer, parameter :: PLUMED_G_INITIALIZED = 1
+      integer :: initialized
+
+      call plumed_f_ginitialized(initialized)


I just added this since I needed it for unit testing, but it's good to have nonetheless.
Now we can call finalize_plumed() safely even before we managed to initalize it (e.g. if ABIN exits early in init.F90).
FYI @suchanj

danielhollas · 2021-01-19T17:30:31Z

+#ifdef USE_PLUMED
+
+   ! This routine is called automatically before each test.
+   @before


This is very useful. See also @after below.

danielhollas · 2021-01-19T17:32:02Z


-   @test(ifdef=SKIPPED_TESTS)
+   @test
+   @disable


An easy way to temporarily disable a unit test.

danielhollas · 2021-01-19T17:36:20Z

Okay, I spent some more quality time with the unit tests for PLUMED and learned a bit more about how pFUnit framework can be used. I'm fairly happy with the result. 🎉

@suchanj I'm going to merge this, but it would be great if you could take a second look at unittests/test_plumed.pf and let me know if it makes sense to you. Also you can try running unit tests locally if you use my make.vars on NEON in /home/hollas/ABIN-DEV. I tried to make comments and and make the tests easy to follow, not sure if I succeeded. Happy to incorporate any feedback in future PR.

danielhollas self-assigned this Jan 11, 2021

danielhollas added the enhancement label Jan 11, 2021

danielhollas force-pushed the plumed-restart branch 4 times, most recently from 3beab6c to 0f44dbe Compare January 13, 2021 17:33

danielhollas commented Jan 13, 2021

View reviewed changes

Comment thread .gitignore Outdated

danielhollas commented Jan 13, 2021

View reviewed changes

danielhollas force-pushed the plumed-restart branch 9 times, most recently from 3c275bd to e1a1333 Compare January 14, 2021 12:15

danielhollas commented Jan 14, 2021

View reviewed changes

Comment thread unit_tests/test_plumed.pf Outdated

danielhollas force-pushed the plumed-restart branch from e1a1333 to 6156224 Compare January 14, 2021 13:58

danielhollas commented Jan 14, 2021

View reviewed changes

Comment thread src/plumed.F90 Outdated

danielhollas commented Jan 14, 2021

View reviewed changes

danielhollas commented Jan 15, 2021

View reviewed changes

Comment thread src/plumed.F90 Outdated

danielhollas requested a review from suchanj January 15, 2021 15:17

danielhollas force-pushed the plumed-restart branch from 6156224 to 49f73ba Compare January 15, 2021 15:17

danielhollas commented Jan 15, 2021

View reviewed changes

danielhollas marked this pull request as ready for review January 15, 2021 15:26

suchanj reviewed Jan 18, 2021

View reviewed changes

suchanj approved these changes Jan 18, 2021

View reviewed changes

danielhollas force-pushed the plumed-restart branch 3 times, most recently from bf24573 to 01da470 Compare January 19, 2021 17:09

danielhollas force-pushed the plumed-restart branch from 01da470 to 9cacb34 Compare January 19, 2021 17:20

danielhollas commented Jan 19, 2021

View reviewed changes

danielhollas merged commit bbcee91 into master Jan 19, 2021

danielhollas deleted the plumed-restart branch January 19, 2021 17:36

Conversation

danielhollas commented Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

danielhollas Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielhollas Jan 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielhollas commented Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danielhollas commented Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielhollas commented Jan 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

danielhollas commented Jan 11, 2021 •

edited

Loading

codecov Bot commented Jan 11, 2021 •

edited

Loading

danielhollas Jan 13, 2021 •

edited

Loading

danielhollas Jan 14, 2021 •

edited

Loading

danielhollas commented Jan 15, 2021 •

edited

Loading

danielhollas commented Jan 15, 2021 •

edited

Loading