Project

General

Profile

Bug #2171

editconf -h indicates -center defaults to (0,0,0) but that is not true from a user perspective

Added by Chris Neale over 2 years ago. Updated over 2 years ago.

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

Description

gmx editconf -c deaults to centering the selected group in the unit cell whose bottom left back corner is at 0,0,0. That's the desired deafult behaviour. However, the -h documentaion is a little confusing.

gmx editconf -h lists "0 0 0" as apparently the "coordinates of the geometrical center":

 -center <vector>           (0 0 0)
           Coordinates of geometrical center

This is not a useful default value as providing "-center" without any numbers gives an error:

GROMACS:      gmx editconf, VERSION 5.1.2
Executable:   /home/cneale/exec/GROMACS/exec/gromacs-5.1.2/serial/bin/gmx
Data prefix:  /home/cneale/exec/GROMACS/exec/gromacs-5.1.2/serial
Command line:
  gmx editconf -f b.gro -n index.ndx -o sel_and_center.gro -c -center

-------------------------------------------------------
Program:     gmx editconf, VERSION 5.1.2
Source file: src/gromacs/commandline/cmdlineparser.cpp (line 234)
Function:    void gmx::CommandLineParser::parse(int*, char**)

Error in user input:
Invalid command-line options
  In command-line option -center
    Too few (valid) values

For more information and tips for troubleshooting, please check the GROMACS
website at http://www.gromacs.org/Documentation/Errors
-------------------------------------------------------

So (0,0,0) is neither a useful default value if "-center" is provided on its own, nor it is the value used for the center when "-center" is not provided at all with -c like this:

gmx editconf -f b.gro -n index.ndx -o sel_and_center.gro -c

It's just a documentation thing. I think the actual performance is what one would desire.

Associated revisions

Revision 36138bbb (diff)
Added by Justin Lemkul over 2 years ago

Clarifications to editconf help text.

It is possible that users can confuse -c with -center so this
patch makes it clear that -center doesn't do anything unless the
user really wants to shift the center of the system away from the
middle of the box.

Fixes #2171

Change-Id: Icd9048b00219ff83eb0297046bb7dfd8e8d7de10

Revision 48353d37 (diff)
Added by Justin Lemkul over 2 years ago

Clarifications to editconf help text.

It is possible that users can confuse -c with -center so this
patch makes it clear that -center doesn't do anything unless the
user really wants to shift the center of the system away from the
middle of the box.

Fixes #2171

Change-Id: Icd9048b00219ff83eb0297046bb7dfd8e8d7de10
(cherry picked from commit 36138bbb4ddcf28080819b11e9302f2825f4ebab)

History

#1 Updated by Justin Lemkul over 2 years ago

Vector specifications have always required either 1 or 3 values, which is mentioned here:

http://manual.gromacs.org/documentation/5.1.2/user-guide/cmdline.html#handling-specific-types-of-command-line-options

Maybe we should change "can" to "must" in the above statement?

#2 Updated by Chris Neale over 2 years ago

Sorry for the confusion. My issue is not that -center without arguments does not work. My issue is this: in what way is the defult value to center = (0,0,0) ? I think it is probably actually the default at some point in the code, but if center is not supplied then the center is not interpreted as (0,0,0) but rather as (x/2, y/2, z/2) for box dimensions x, y, z. I only pointed out that center without arguments does not work in order to complete the logical proof that (0,0,0) is not a default value for -center from a user perspective. You could just as easly re-write the editconf -h output to say

-center <vector>           (-999 -999 -999)

and it would be no more false than the current output of editconf -h (unless I'm missing something, of course).

#3 Updated by Justin Lemkul over 2 years ago

Indeed, (-999 -999 -999) is no more false, but why change it if it doesn't make it more true? The only real way to make this obvious is to change the default value to ( uninitialized uninitialized uninitialized ) but that is impossible from a code perspective and impractical from a printing perspective. So there is indeed a problem in how we document essentially uninitialized quantities, and there are places in the code where the default of a vector is sensible (e.g. genconf -nbox) but other elements even in editconf where the (0 0 0) is valid. I think listing nonsensical values is even more confusing, honestly. The documentation says vectors require 1 or 3 values; the code performs as such so if you abuse a vector option the command simply fails. In this specific instance, it's just a somewhat-awkward interpretation of a clumsy placeholder value. But it is documented correctly and performs in a manner consistent with that documentation, so this isn't really a bug.

#4 Updated by Chris Neale over 2 years ago

Agreed changing it to -999's is a terrible idea. That was my (clearly bad) way of trying to clarify my point. If people perfer to leave it as it is then that's fine with me. While I do try to post even trivial bugs/suggestions whenever I run into them, I'm not one to go out of my way to search the documentaion for the purpose of making inane suggestions. This is something that bit me in an automated workflow (I use amber a lot now so I am less familiar with the gromacs behaviour and I rely on -h more than I used to). So I checked the edticonf -h saw that the default was to put the center at 0,0,0 and wrote my script with the -c option and without the -center option. Things broke and I tracked it down to this as a source of confusion.

I actually really like your "uninitalized" suggestion in combination with some header text describing the selection of the center when -c is given without -center. I don't honestly see how anything is impossible from a code perspective, but if the argument is that the necessary changes would be either too much work for the small gain or would be more likely to introduce other bugs / not be used properly in the future then I am fine with that too.

At this point, I leave it to Justin and others more familiar with the code to make the decision. I'm fine if you want to close and reject this issue.

Thank you,
Chris.

#5 Updated by Justin Lemkul over 2 years ago

  • Status changed from New to Rejected

There's a more general problem. You're essentially expecting -center to act as a boolean, which it is not. The problem you're reporting happens with any command-line option that is not provided a value when one is required. gmx editconf -f test.pdb -o test.gro -c -d similarly fails because -d needs an actual argument. It doesn't simply take the default value of 1.0 and work. Options are enabled when other options require them to, in which case, they behave as a "black box" to work (e.g. gmx genconf -f test.gro -o test.pdb defaults to a single box via -nbox 1 1 1 without the user doing anything, but gmx genconf -f test.gro -o test.pdb -nbox simply fails because it's wrong syntax). If a user specifies a given option, it has to be correctly specified.

So I think it's best to just close this issue.

#6 Updated by Chris Neale over 2 years ago

OK, but I think we're still talking about different things. I don't care that -center failes without an argument. What I care is that:

gmx editconf -f in.gro -n index.ndx -o out.gro -c

will center the box at x/2, y/2, z/2 while

gmx editconf -h

indicates the default center is 0,0,0 not x/2,y/2,z/2.

#7 Updated by Gerrit Code Review Bot over 2 years ago

Gerrit received a related patchset '1' for Issue #2171.
Uploader: Justin Lemkul ()
Change-Id: gromacs~master~I6696dfe6f1226efaf8f2414c1a291332487f4f3b
Gerrit URL: https://gerrit.gromacs.org/6610

#8 Updated by Justin Lemkul over 2 years ago

  • Status changed from Rejected to Feedback wanted

#9 Updated by Chris Neale over 2 years ago

Not sure what "feedback wanted" means. Is it feeback from me? In that case I did try to follow the gerit link but I can not see it as a guest. If feedback is wanted from me, then is there a way that I can see the gerrit review without a login?

#10 Updated by Justin Lemkul over 2 years ago

Looks like Gerrit ate the uploaded patch. I'll see if I can figure out what's going on, but yes, ultimately I want your feedback on the patch to see if it addresses the issue. You'll be able to see the code changes without logging into anything once I get the commit to show back up in the system.

#11 Updated by Gerrit Code Review Bot over 2 years ago

Gerrit received a related patchset '1' for Issue #2171.
Uploader: Justin Lemkul ()
Change-Id: gromacs~master~Icd9048b00219ff83eb0297046bb7dfd8e8d7de10
Gerrit URL: https://gerrit.gromacs.org/6622

#12 Updated by Justin Lemkul over 2 years ago

New patch uploaded. Not sure why Gerrit ate my last one, but hopefully this one sticks!

#13 Updated by Chris Neale over 2 years ago

Thank you Justin. However, it is unclear to me how this code change solves the issue. I think a better text output would be:

-center <vector>           (0 0 0)
           Shift the geometrical center to (x,y,z). When -c is used without -center, the geometrical center is shifted to (x/2, y/2, z/2) for box dimensions (x,y,z).

If that's too much text for this part of the output, perhaps a sentence like that could go in the preamble part of the -h output?

#14 Updated by Justin Lemkul over 2 years ago

Yes, that's too much text and we should not be explaining what -c does in the help text for -center. That's even more confusing. The behavior of -c is described in the main description but -center is not, so I am going to make modifications largely to the help text to explain -center. I will upload a new commit momentarily.

#15 Updated by Justin Lemkul over 2 years ago

I'm not sure why the new patch set is not showing up here, but I've modified the text so please refer to the existing link and see patch set 5.

#16 Updated by Chris Neale over 2 years ago

Looks good. Thanks Justin.

#17 Updated by Gerrit Code Review Bot over 2 years ago

Gerrit received a related patchset '1' for Issue #2171.
Uploader: Mark Abraham ()
Change-Id: gromacs~release-2016~Icd9048b00219ff83eb0297046bb7dfd8e8d7de10
Gerrit URL: https://gerrit.gromacs.org/6629

#18 Updated by Justin Lemkul over 2 years ago

  • Status changed from Feedback wanted to Resolved

#20 Updated by Erik Lindahl over 2 years ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF