Differences in output between GIST and `grid`

[drroe]

From Thomas Fox via the Amber mailing list:

I think I stumbled over another inconsistency w.r.t. grid calculations.

With the following cpptraj input: parm 2JAI_Mod_SolvIon_4Amber.top trajin 2JAI.nc parmbox 0. 0. 0. grid grid.dx 4 1. 4 1. 4 1. gridcenter 20. -2. 50. :WAT@O bincenter gist gridcntr 20. -2. 50. griddim 4 4 4 gridspacn 1. out gist.out

I get two dx files for the water oxygen density, grid.dx (from the grid command) and gist-gO.dx (from the gist command) which I thought should produce a very similar output, as in both cases I calculate the same quantity.

However, the dx headers read: grid.dx : origin 18.5 -3.5 48.5 gist-gO.dx : origin 17.5 -4.5 47.5

While the grid box definition is how I would expect it, the gist origin seems to be off by half a grid spacing. I would expect as x-values either 18,19,20,21,22 ( in case griddim refers to the number of grid bins - this is how I would understand the manual as it says "Integer number of grid increments along each coordinate axis") or 18.5., 19.5, 20.5, 21.5 (if grddim refers to the number of grid points in each direction which needs to shift by half a spacing to make up for the even number of points = uneven number of bins).

Consequently, when I plot the two grids, first of all the grid boxes are shifted, but I also see differences in the grids; and more differences that I would have expected. For a single frame, this means that in some cases the two minima are on the same grid point, sometimes they are shifted into one direction in some cases in two directions by one grid point. For multiple frames the "structure" of the resulting grid maps looks very different - some of the contours (especially those with high occupancy) are in similar areas, but many of the contours appear at very different locations - is this to be expected?

One more thing: In my above example, the trajectory has already been preprocessed, e.g. imaged and superposed. Yet, gist insists on getting box information, even if it is as useful as the command parmbox 0. 0. 0.

[drroe]

@cnnguyen @gosldorf Pinging you guys for easy reference. Also #199 is related to this.

[cnnguyen]

Hi Thomas, it's possible that Gist produces an output grid with an origin shifted by half a grid spacing. I think the way I had it implemented is correct for grid with odd number of grid dimensions. Can you try a grid dimension of 5x5x5 instead of 4x4x4 and see if Gist's origin and Grid's origin become consistent? Thanks

[foxth]

Hi Crystal, yes, when I use even grid dimensions N in grid (grid needs even numbers here) and (N-1) grid points per dimension in gist I indeed get two dx file which have identical origins, but 1) I get different grids that are not easy to compare, 2) there is still this inconsistency wrt grid steps vs. grid points that we had in the grid command, and 3) visualizing the grids for one single frame, the contours of the grid obtained with the bincenter command match best to the actual water coordinates (compared to the "old" grid command and the gist grid).

Best, Th.

[drroe]

@gosldorf I didn't notice this before but the GIST output was actually already changed by your previous PR #196. The differences are in dTSorient-dens, dTSorient-norm, dTStrans-dens, and dTStrans-norm. For example:

voxel 8310 8310
  dTStrans-dens(kcal/mol/A^3) -3.69002 -3.6948
  dTStrans-norm(kcal/mol) -2.30626 -2.30925
  dTSorient-dens(kcal/mol/A^3) 1.31001 2.31939
  dTSorient-norm(kcal/mol) 0.818755 1.44962

I will attach the full diffs. GistDiffs.txt We need to make sure the new output is legitimate. In the meantime I am going to start a PR that enables all tests by default to avoid this happening in the future.

[gosldorf]

Ah. Actually that makes sense. So the changes I made to the code in that earlier PR had modified the way translational and orientational entropy were calculated as well as adding combined entropies, therefore those data points being different is expected. I've extensively tested the formulations I put in and know that they behave as they should...but they won't pass the similarity test. If the diffs for the energies are also different then that's very strange since I had not touched that part of the code, however.

I remade the test cases based on my current code, wherein the grid origin is repositioned and the newer entropic formulations are in use, last Friday, but have not pushed those files. Crystal and I discussed the grid origin issue and came to the conclusion that new test files were necessary based on every line being different (due to the grid being shifted), but in hindsight I need new test files to account for the entropic modifications as well.

To summarize: Gist grid has been shifted to respond to Thomas Fox's correct assertion that the position of the origin was unintuitive and I have changed the formulation for all entropies since the previous release. Both modifications will make the code fail the comparison with old test files, however I'm confident that the entropies I'm adding and the grid origin position are solid.

Sorry for the confusion/ late response/ lengthy response, if there's something I can do to alleviate the uncertainty I'd be glad to do so, thanks for the help,

--Steve

On Fri, Dec 11, 2015 at 4:05 PM, Daniel R. Roe notifications@github.com wrote:

@gosldorf https://github.com/gosldorf I didn't notice this before but the GIST output was actually already GistDiffs.txt https://github.com/Amber-MD/cpptraj/files/59833/GistDiffs.txt changed by your previous PR #196 https://github.com/Amber-MD/cpptraj/pull/196. The differences are in dTSorient-dens, dTSorient-norm, dTStrans-dens, and dTStrans-norm. For example:

voxel 8310 8310 dTStrans-dens(kcal/mol/A^3) -3.69002 -3.6948 dTStrans-norm(kcal/mol) -2.30626 -2.30925 dTSorient-dens(kcal/mol/A^3) 1.31001 2.31939 dTSorient-norm(kcal/mol) 0.818755 1.44962

I will attach the full diffs. We need to make sure the new output is legitimate. In the meantime I am going to start a PR that enables all tests by default to avoid this happening in the future.

— Reply to this email directly or view it on GitHub https://github.com/Amber-MD/cpptraj/issues/198#issuecomment-164047282.

[cnnguyen]

Hi all

The shift in the origin should also modify the energy values even though there is no change in the energy formulation. I think the new test case should be good. Thanks

Crystal

On Dec 14, 2015, at 7:43 AM, Steven Ramsey notifications@github.com<mailto:notifications@github.com> wrote:

Ah. Actually that makes sense. So the changes I made to the code in that earlier PR had modified the way translational and orientational entropy were calculated as well as adding combined entropies, therefore those data points being different is expected. I've extensively tested the formulations I put in and know that they behave as they should...but they won't pass the similarity test. If the diffs for the energies are also different then that's very strange since I had not touched that part of the code, however.

I remade the test cases based on my current code, wherein the grid origin is repositioned and the newer entropic formulations are in use, last Friday, but have not pushed those files. Crystal and I discussed the grid origin issue and came to the conclusion that new test files were necessary based on every line being different (due to the grid being shifted), but in hindsight I need new test files to account for the entropic modifications as well.

To summarize: Gist grid has been shifted to respond to Thomas Fox's correct assertion that the position of the origin was unintuitive and I have changed the formulation for all entropies since the previous release. Both modifications will make the code fail the comparison with old test files, however I'm confident that the entropies I'm adding and the grid origin position are solid.

Sorry for the confusion/ late response/ lengthy response, if there's something I can do to alleviate the uncertainty I'd be glad to do so, thanks for the help,

--Steve

On Fri, Dec 11, 2015 at 4:05 PM, Daniel R. Roe notifications@github.com<mailto:notifications@github.com> wrote:

@gosldorf https://github.com/gosldorf I didn't notice this before but the GIST output was actually already GistDiffs.txt https://github.com/Amber-MD/cpptraj/files/59833/GistDiffs.txt changed by your previous PR #196 https://github.com/Amber-MD/cpptraj/pull/196. The differences are in dTSorient-dens, dTSorient-norm, dTStrans-dens, and dTStrans-norm. For example:

voxel 8310 8310 dTStrans-dens(kcal/mol/A^3) -3.69002 -3.6948 dTStrans-norm(kcal/mol) -2.30626 -2.30925 dTSorient-dens(kcal/mol/A^3) 1.31001 2.31939 dTSorient-norm(kcal/mol) 0.818755 1.44962

I will attach the full diffs. We need to make sure the new output is legitimate. In the meantime I am going to start a PR that enables all tests by default to avoid this happening in the future.

— Reply to this email directly or view it on GitHub https://github.com/Amber-MD/cpptraj/issues/198#issuecomment-164047282.

— Reply to this email directly or view it on GitHubhttps://github.com/Amber-MD/cpptraj/issues/198#issuecomment-164471124.

[drroe]

OK, @gosldorf can you update your PR #199 with the updated test case files? Once that is done I can merge, and then I will then start a PR which enables the GIST test by default.

Also, @cnnguyen do you think there is a better or additional test case that should be run or is the existing one OK?

[cnnguyen]

Hi Dan

I think just updating the existing test case should be okay. Thanks

Crystal

On Dec 14, 2015, at 8:33 AM, Daniel R. Roe notifications@github.com<mailto:notifications@github.com> wrote:

OK, @gosldorfhttps://github.com/gosldorf can you update your PR #199https://github.com/Amber-MD/cpptraj/pull/199 with the updated test case files? Once that is done I can merge, and then I will then start a PR which enables the GIST test by default.

Also, @cnnguyenhttps://github.com/cnnguyen do you think there is a better or additional test case that should be run or is the existing one OK?

— Reply to this email directly or view it on GitHubhttps://github.com/Amber-MD/cpptraj/issues/198#issuecomment-164486899.

[gosldorf]

Hey all,

I believe I've updated PR 199, in that I've pushed the updated .save files and they appear in the notes when I follow the PR link. Please let me know if I need to do an additional step to finalize that PR modification.

Thanks for all the help!

--Steve

On Mon, Dec 14, 2015 at 1:24 PM, cnnguyen notifications@github.com wrote:

Hi Dan

I think just updating the existing test case should be okay. Thanks

Crystal

On Dec 14, 2015, at 8:33 AM, Daniel R. Roe <notifications@github.com mailto:notifications@github.com> wrote:

OK, @gosldorfhttps://github.com/gosldorf can you update your PR #199< https://github.com/Amber-MD/cpptraj/pull/199> with the updated test case files? Once that is done I can merge, and then I will then start a PR which enables the GIST test by default.

Also, @cnnguyenhttps://github.com/cnnguyen do you think there is a better or additional test case that should be run or is the existing one OK?

— Reply to this email directly or view it on GitHub< https://github.com/Amber-MD/cpptraj/issues/198#issuecomment-164486899>.

— Reply to this email directly or view it on GitHub https://github.com/Amber-MD/cpptraj/issues/198#issuecomment-164517338.

[drroe]

Thanks again everyone - looks good to merge.