Project

General

Profile

Bug #1294

Separate LR non-bonded dvdlambda term in the log is incorrect

Added by Teemu Murtola over 4 years ago. Updated about 4 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
mdrun
Target version:
Affected version - extra info:
Affected version:
Difficulty:
uncategorized
Close

Description

In sim_util.c, around line 1600, dvdlambda is passed to fprintf to be printed as the "LR non-bonded" term as if it was a single value, but it's actually an array. So garbage gets printed. Found while resolving some C++ compilation warnings in master.

Associated revisions

Revision ba223d53 (diff)
Added by Teemu Murtola over 4 years ago

Switch remaining main() functions and copyrite.c to C++.

Required to be able to use gmx::ProgramInfo from these (done in
separate commit), and simplifies the compilation of mdrun as well.

Resolved compiler/cppcheck/clang warnings that were present in these
files. Some include file clean-up, motivated by the fact that force.h
causes warnings when included in C++ files, and fixing that requires
fixing #1294, where the solution is not yet decided on.

Change-Id: I69f4200c1c1783340d88381a6aca82151d1230cb

Revision da8b25b6 (diff)
Added by Teemu Murtola over 4 years ago

Pre-emptively fix some C++ warnings.

Moved sepdvdlformat from force.h into a source file to avoid warnings
about an unused variable. These appeared as soon as a C++ source file
included force.h. As an additional benefit, makes printing this
information more typesafe; this is how #1294 was found.

Change-Id: Id856fbe193d9b4a74ecfb49cba9eab03bad0a7ba

Revision 0d56607f (diff)
Added by Mark Abraham over 4 years ago

Removed buggy -seppot output

ns() no longer computes forces, so the comment needs correcting.

dvdlambda was changed to an array at some point, but the output code
did not change in sync. So, that output has been broken in at least
4.6, and is always zero anyway. So the output code can go away.

A patch in master branch removes the useless parameters in the
neighbour-search code. When merging this patch into master branch,
eliminate the useless dvdlambda variable.

Fixes #1294

Change-Id: Iba48f549ee07f0ee1877035a9e506780439c8002

History

#1 Updated by Michael Shirts over 4 years ago

Good point. Probably got broken because not many people are using these features.

So, is anyone using these features anymore? At this point, many separate components can be accessed if you just request the components to be printed out separately. So it seems like these code lines (triggered by bSepDVDL) should either be removed, or changed to debugging flag only.

Thoughts?

#2 Updated by Teemu Murtola over 4 years ago

I'm all for removing this if it isn't necessary any longer. Are you saying that you can get more or less the same output in the edr file nowadays? In that case, I don't see why we would need this log output. Even if it remains as debug output, it still needs to be fixed. But I don't have that much experience with mdrun (in particular with free energy calculations), and don't use it myself any longer, so I can't really make the call.

Although this is probably not very high priority since people are probably not using it, it still potentially blocks some things in master (can't make things compile without changing something here), and I'd prefer to either fix it or remove the code ASAP. If we can't remove it completely, should it print the sum of the efptCOUL and efptVDW terms from the array here, or something else? Should it be fixed/removed also in 4.6.x, or is it enough to resolve this for 5.0?

#3 Updated by Mark Abraham over 4 years ago

I've never heard of anyone using it. Seems to me mostly like it's debugging output. If so, then we should trigger it via -debug. Perhaps it doesn't do that because the -debug runs (and files) are such a PITA to use. We might have to wait until Berk is back for an explanation.

For the moment for master branch, I would make it write only if -debug is on, and write to the log file (as now). Writing the sum of efptCOUL and efptVDW at that point makes sense - and matters only if twin-range neighbour searching is active.

Since there's no bug filed against 4.6, I wouldn't bother to fix it there while the correct solution remains unclear.

#4 Updated by Teemu Murtola over 4 years ago

https://gerrit.gromacs.org/#/c/2474/ seems to reveal an additional problem with this same line: it appears that the neighborhood search no longer does any force computation, so it doesn't compute any dvdhlambda terms either. So even if we would print the sum term here, it would probably always be zero.

#5 Updated by Mark Abraham over 4 years ago

Teemu Murtola wrote:

https://gerrit.gromacs.org/#/c/2474/ seems to reveal an additional problem with this same line: it appears that the neighborhood search no longer does any force computation, so it doesn't compute any dvdhlambda terms either. So even if we would print the sum term here, it would probably always be zero.

Indeed, that happened in 9487b95114ca3c579fa4302c2c96e0b1a51b0f0f. So this whole fragment can go away in 4.6 (where there is a bug).

Also lambda and dvdlambda are not used in ns(). I will also patch master branch to eliminate the unused parameters (which is a separate issue, and does not need to be fixed in 4.6).

#6 Updated by Mark Abraham over 4 years ago

  • Status changed from New to Fix uploaded
  • Assignee changed from Berk Hess to Mark Abraham
  • Target version set to 4.6.4

Mark Abraham wrote:

Also lambda and dvdlambda are not used in ns(). I will also patch master branch to eliminate the unused parameters (which is a separate issue, and does not need to be fixed in 4.6).

I see Alexey has already done this as part of https://gerrit.gromacs.org/2474

#7 Updated by Mark Abraham over 4 years ago

Teemu Murtola wrote:

I'm all for removing this if it isn't necessary any longer. Are you saying that you can get more or less the same output in the edr file nowadays? In that case, I don't see why we would need this log output. Even if it remains as debug output, it still needs to be fixed. But I don't have that much experience with mdrun (in particular with free energy calculations), and don't use it myself any longer, so I can't really make the call.

Although this is probably not very high priority since people are probably not using it, it still potentially blocks some things in master (can't make things compile without changing something here), and I'd prefer to either fix it or remove the code ASAP. If we can't remove it completely, should it print the sum of the efptCOUL and efptVDW terms from the array here, or something else? Should it be fixed/removed also in 4.6.x, or is it enough to resolve this for 5.0?

I briefly talked to Berk about this yesterday. It is pretty much debugging output. mdrun -seppot is supposed to trigger a per-MPI-rank .log file + local output, because that was sometimes useful back in the PD days (but Berk was not sure it still works properly). It's been hijacked to do this kind of dvdlambda output too. So there will probably be a need to preserve the general functionality (if it works), but perhaps the trigger needs some re-design.

#8 Updated by Mark Abraham over 4 years ago

  • Status changed from Fix uploaded to Resolved
  • % Done changed from 0 to 100

#9 Updated by Teemu Murtola about 4 years ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF