WIP: MetaD NEIGHBOR list option to sum gaussians #639
Conversation
this can be usefull with many cvs where grids are either too memory consuming and/or too slow. I was playing with this idea with OPES and it actually works pretty well. The neighbor list update at the moment is only openMP parallelised. this commits add also the openMP parallelisation to the cycle over the gaussians
threads for few hills
|
This pull request introduces 2 alerts when merging 3ae9fac into 4cc8ce7 - view on LGTM.com new alerts:
|
|
This pull request introduces 2 alerts when merging c35c4aa into 597647b - view on LGTM.com new alerts:
|
|
@carlocamilloni I didn't check the code, but I was wondering if this is exact or with some approximation. If the criterion is on the distance from the position where the list was updated and it is exact, you might also remove the replica exchange condition, right? Then, if it is exact, I would set it as a default and just allow to remove it for debugging. It might even be an environment variable and we would use it in the regtests as a reference. It looks something very useful, as it is will be easier than grids to use, and I saw many people doing the mistake of forgetting using grids. Thanks! |
|
This pull request introduces 3 alerts when merging d835bd6 into 329636b - view on LGTM.com new alerts:
|
|
This pull request introduces 3 alerts when merging ef3049e into 329636b - view on LGTM.com new alerts:
|
Codecov Report
@@ Coverage Diff @@
## master #639 +/- ##
==========================================
+ Coverage 85.58% 85.71% +0.12%
==========================================
Files 579 580 +1
Lines 49672 50350 +678
==========================================
+ Hits 42514 43158 +644
- Misses 7158 7192 +34
Continue to review full report at Codecov.
|
|
This pull request introduces 3 alerts when merging 0d570f4 into 415dce9 - view on LGTM.com new alerts:
|
|
@GiovanniBussi it is not exact, it may be made exact for gaussians with fixed sigma, otherwise it will be in general an approximation. Furthermore, while being faster than plain MetaD is not faster than using GRID because wrt OPES in METAD the number of gaussian increases linearly with time. Anyway I think that for METAD in 3-5 CVs it is the method of choice and it make it feasible. The pull request also add a general, quite significant, speed up to the simple METAD with no grid and neighbour list and an initial cleaning. Gaussian summation is also openMP parallelised now. At the moment I think that the main source of error in using NLIST is the discontinuity at DP2CUTOFF. Tomorrow I will add some number about the performances. |
|
Numbers on ALADP (scalar) Plumed.dat: Original numbers (master branch) New numbers (pull request): note: the performances of GRID are constant, while those of SIMPLE and NLIST decreases with time, the pull request reduces the slope of the drop of performances quite significantly in the SIMPLE case and even more by using NLIST. For GRID and SIMPLE the gain is almost completely associated with a more efficient update(), while NLIST has an impact on calculate(). |
|
I still need to add some regtest and port some more of the work done on OPES |
|
This pull request introduces 3 alerts when merging a0d92fd into a2fc8b4 - view on LGTM.com new alerts:
|
|
|
||
| #define DP2CUTOFF 6.25 | ||
|
|
||
| using namespace std; |
There was a problem hiding this comment.
@carlocamilloni good choice removing this... Code is more verbose but way more readable
|
@carlocamilloni is this also the right moment to transition to smoother functions? (see #420) |
|
@GiovanniBussi it depends whether we want just a local change or if we want to change it all over plumed, in the latter case it should be a separate pull request. Anyway I think this is rather important for NLIST because right now it is the largest source of error.
… On 17 Nov 2020, at 09:25, Giovanni ***@***.***> wrote:
@carlocamilloni <https://github.com/carlocamilloni> is this also the right moment to transition to smoother functions? (see #420 <#420>)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#639 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABC6L5VSMRPDUXV33F6W5JTSQIXNZANCNFSM4TOMIDMQ>.
|
|
@GiovanniBussi about the namespace I would forcibly remove it all over plumed, or at least I would propose to begin a transition in this direction
… On 17 Nov 2020, at 09:17, Giovanni ***@***.***> wrote:
@GiovanniBussi commented on this pull request.
In src/bias/MetaD.cpp <#639 (comment)>:
>
#define DP2CUTOFF 6.25
-using namespace std;
@carlocamilloni <https://github.com/carlocamilloni> good choice removing this... Code is ore verbose but way more readable
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#639 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABC6L5WM4DFQILHFDHCXNBLSQIWRJANCNFSM4TOMIDMQ>.
|
Yes I agree... Another PR to be opened after this is merged |
|
Currently we forbid it in header files, since that is a very bad practice
(files including that header will inadvertently use the namespace). It
would be easy to forbid them everywhere, though there would be some work
required:
```
git grep "using namespace std" | grep -v lapack | grep -v blas | grep -v
lepton | grep -v asmjit | less | wc -l
173
```
On Tue, Nov 17, 2020 at 9:29 AM Carlo Camilloni <notifications@github.com>
wrote:
… @GiovanniBussi about the namespace I would forcibly remove it all over
plumed, or at least I would propose to begin a transition in this direction
> On 17 Nov 2020, at 09:17, Giovanni ***@***.***> wrote:
>
>
> @GiovanniBussi commented on this pull request.
>
> In src/bias/MetaD.cpp <
#639 (comment)>:
>
> >
> #define DP2CUTOFF 6.25
>
> -using namespace std;
> @carlocamilloni <https://github.com/carlocamilloni> good choice
removing this... Code is ore verbose but way more readable
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub <
#639 (review)>,
or unsubscribe <
https://github.com/notifications/unsubscribe-auth/ABC6L5WM4DFQILHFDHCXNBLSQIWRJANCNFSM4TOMIDMQ
>.
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#639 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABBNCXT6QGGZUKCR45GJN2LSQIX5LANCNFSM4TOMIDMQ>
.
|
|
Yes, I would do it from master so that it is done for version 2.8
In refactoring metad I found it helpful also to force me to better understand the code
… On 17 Nov 2020, at 09:39, Giovanni ***@***.***> wrote:
Currently we forbid it in header files, since that is a very bad practice
(files including that header will inadvertently use the namespace). It
would be easy to forbid them everywhere, though there would be some work
required:
```
git grep "using namespace std" | grep -v lapack | grep -v blas | grep -v
lepton | grep -v asmjit | less | wc -l
173
```
On Tue, Nov 17, 2020 at 9:29 AM Carlo Camilloni ***@***.***>
wrote:
> @GiovanniBussi about the namespace I would forcibly remove it all over
> plumed, or at least I would propose to begin a transition in this direction
>
> > On 17 Nov 2020, at 09:17, Giovanni ***@***.***> wrote:
> >
> >
> > @GiovanniBussi commented on this pull request.
> >
> > In src/bias/MetaD.cpp <
> #639 (comment)>:
> >
> > >
> > #define DP2CUTOFF 6.25
> >
> > -using namespace std;
> > @carlocamilloni <https://github.com/carlocamilloni> good choice
> removing this... Code is ore verbose but way more readable
> >
> > —
> > You are receiving this because you were mentioned.
> > Reply to this email directly, view it on GitHub <
> #639 (review)>,
> or unsubscribe <
> https://github.com/notifications/unsubscribe-auth/ABC6L5WM4DFQILHFDHCXNBLSQIWRJANCNFSM4TOMIDMQ
> >.
> >
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#639 (comment)>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ABBNCXT6QGGZUKCR45GJN2LSQIX5LANCNFSM4TOMIDMQ>
> .
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#639 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABC6L5XQUJXFLH6KA5PNJOTSQIZCPANCNFSM4TOMIDMQ>.
|
|
This pull request introduces 3 alerts when merging 99cd349 into a2fc8b4 - view on LGTM.com new alerts:
|
|
This pull request introduces 3 alerts when merging 36ecc51 into c5c6ba9 - view on LGTM.com new alerts:
|
from which reading GRID or HILLS files upon RESTART
|
This pull request introduces 3 alerts when merging 58c71c2 into 2a40493 - view on LGTM.com new alerts:
|
|
This pull request introduces 3 alerts when merging 2091eab into 9b53458 - view on LGTM.com new alerts:
|
|
This pull request introduces 3 alerts when merging 241a475 into 9b53458 - view on LGTM.com new alerts:
|
|
This pull request introduces 3 alerts when merging 66d6ced into a4456d9 - view on LGTM.com new alerts:
|
this can be usefull with many cvs where grids are either too memory consuming
and/or too slow. I was playing with this idea with OPES and it actually works
pretty well. The neighbor list update at the moment is only openMP parallelised.
this commits add also the openMP parallelisation to the cycle over the gaussians.
The neighbour list is updated:
@GiovanniBussi @maxbonomi
I also need to add and perform some test