Navigation Menu

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

Fix: Segmentation fault whenever more than 2 CVs are used for METAD w… #399

Closed
wants to merge 1 commit into from

Conversation

fiskissimo
Copy link
Contributor

@fiskissimo fiskissimo commented Oct 17, 2018

Description

Bugfix: Segmentation fault whenever more than 2 CVs are used for METAD with REWEIGHTING_NGRID #328

Target release

I would like my code to appear in release 2.5

Type of contribution
  • changes to code or doc authored by PLUMED developers
  • changes to a module not authored by you
  • new module contribution or edit of a module authored by you
Copyright
  • I agree to transfer the copyright of the code I have written to the PLUMED developers or to the author of the code I am modifying.
  • the module I added or modified contains a COPYRIGHT file with the correct license information. I also used the command cd src && ./header.sh mymodulename in order to make sure the headers of the module are correct.
Tests
  • I added a new regtest or modified an existing regtest to validate my changes.
  • I verified that all regtests are passed successfully on Travis-CI.

…ith REWEIGHTING_NGRID plumed#328

I tried to interpret and fix the code and for me it works nicely with 1 to 3 CVs. 
Here a brief description: the index i spans through the whole reweghting-grid (according to the mpi rank in case of mpi).
rewf_grid_ contains the size on each dimension of the reweghting-grid. t_index is the array containing the coordinates of i in each dimension of the grid. The indexes had to be changed from i to j in line 1967, which indeed spans correctly the dimensions of the grid. In 1968 in order to get the correct index, we need to subtract the former index (ncv-2 -- instead of "ncv-1" which is yet not assigned -- from what is left of kk).

I didn't attempt any further test.

Please let me know if I had misinterpreted the code.
@GiovanniBussi
Copy link
Member

@valsson I think you first implemented these lines. Could you double check the change?

If this is correct, I guess it should be backported to 2.4 at least (and perhaps 2.3, though we just declared it as non-supported).

@valsson
Copy link
Contributor

valsson commented Oct 18, 2018

There is for sure some bug with the c(t) calculation for 3 or more CVs that has to be fixed.

Now, it is actually @gtribello that implemented the integration where the bug/fix is. So it is better if he would take a look at the exact changes.

Furthermore, I think it would be safer to change the unsigned int on lines 1964 and 1966 to Grid::index_t.

Looking a travis, is seems that the change breaks the regtest (rt67) for 2 CVs, there are some small numerical difference.

@gtribello
Copy link
Member

Hello all

Sorry for taking a while to look at this. The code definitely looks like it is a proper fix for what is there. I am happy to merge this pull request.

Thanks

@GiovanniBussi GiovanniBussi self-assigned this Dec 17, 2018
@GiovanniBussi
Copy link
Member

Since this is a bug fix, I'll merge it to v2.3 and fix the necessary regtests

GiovanniBussi added a commit that referenced this pull request Dec 17, 2018
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.

None yet

4 participants