Nexus: Update qdens command-line argument parsing - #6213
Conversation
There was a problem hiding this comment.
TLDR
Things are not all well here. Tests pass, but the test coverage is not sufficient to support this kind of rewrite without first writing some tests.
One way around this would be to first drop functionality deleting complex handling of things that should be coming from the input.xml. Then add testing to coverage for what is left.
Then port to ArgumentParser
The testing that existed and that I added when I touched qdens was just to defend the functionality I wanted and assure me I wasn't breaking things as I made that fix, it isn't a sufficient to support this because it all still works.
I consider myself a nexus tourist so I didn't just write "Where is testing?"as an initial review. I was inclined to stamp this good enough since my tests still pass so the batched pathway I care about appears unbroken. And deprecated OptionParser and hand tooled parsing?
But I pulled the branch and made a worktree and and started up the same sort of agentic review with gpt-5.6-luna,terra, and sol. I won't subject anyone to the direct output of that today but...
It was quickly clear the argument parsing behavior is significantly changed. And to me unexpectedly. Initially this is what I cared about because it was very close breaking what I'd recently done.
What I didn't want to see broken from my last qdens works was invocations like
qdens -f xsf -i input.xml file.stat.h5
and that still works but that is by chance
qdens -i input.xml -f xsf file.stat.h5
Now fails.
They look equally valid and it feels like it could become my problem.
Things get fiddly mixing greedy flags with positional arguments. A user needs to know which flags are greedy now and which aren't, most POSIX environment cli program flags are not greedy for a reason.
Another part to skip here but basically its a usability regression I don't personally like
The comma syntax previously used with -f is a common unix style argument handling convention and I'd say without support way more common than greedy flags which is what nargs="*" makes. The case that is in the tests still works because the -i terminates the -f's greedy consumption of arguments.
POSIX rules for shells and arguments say xsf,dat is one operand so this is a cute way to have multiple arguments with uniform flag behavior. But the application has to parse that operand its not the shells problem. I wouldn't think the old utility writers cared about user experience but they chose to do some of their own parsing rather than make users keep track of two different flag behaviors.
So that's something nice that comma list thing did.
my formats issue could be solved by something like
from argparse import ArgumentTypeError
def formats_arg(value):
formats = value.replace(",", " ").split()
allowed = {"dat", "xsf", "chgcar"}
invalid = set(formats) - allowed
if invalid:
raise ArgumentTypeError(f"unknown format(s): {sorted(invalid)}")
return formats
parser.add_argument("-f", "--formats", type=formats_arg)
Which would leave -f with behavior I'd find unsurprising since most of the time I expect only one format is going to be used.
It does have an error and stop and you can figure out from the error that -f ate your h5 file with a bit of thought.
But I buried the lead because I think the input.xml should be the single source of truth. And I'd like the usability preserved.
I only specifically point out one issue with cell but I believe other options like --density-cell and --density-corner no longer work as they did, to the extent where I think they are only going to not work as intended or cause an immediate failure.
| ) | ||
| raise ValueError(msg) | ||
| #end if | ||
| cell = np.array(cell,dtype=float) |
There was a problem hiding this comment.
Isn't cell still None so we're tossing the opt.cell input away.
Writing testing to cover what is being rewritten would be a better aproach.
At this point I have to say I need to see some testing written or I can't approve.
3 participants
Proposed changes
This PR updates the argument parsing for
qdensto useargparseand reduce custom input handling code.Some changes include delegating type handling of arguments to
argparse, delegating argument length handling toargparse, and simplifying the input format.Changes and Removals
This PR removes the ability to pass a dict into the input, which was undocumented, untested, and relied on the
evalfunction in Python, which can lead to obscure errors. Also, the previous version would allow passing--inputalong with--structure/--cell, which would silently overwrite any structure/cell in the input file with the one passed through--structure/--cell. Due to the fact that the input must have these elements, it is likely undesirable behavior, so the arguments are now mutually exclusive.This PR additionally changes the formatting that was used for list-like arguments, which was originally like
--grid "3 3 3"and is now--grid 3 3 3, whichargparsewill automatically parse into a list of 3 integers.What type(s) of changes does this code introduce?
Does this introduce a breaking change?
What systems has this change been tested on?
Laptop, Fedora Linux 44 (KDE Plasma Desktop Edition)
AMD Ryzen 7 PRO 7840U (8 cores, 16 logical processors)
Checklist