sxs-collaboration / spectre

SpECTRE is a code for multi-scale, multi-physics problems in astrophysics and gravitational physics.
https://spectre-code.org
Other
153 stars 185 forks source link

spectre plot slice usability improvements #6085

Closed nilsdeppe closed 3 weeks ago

nilsdeppe commented 3 weeks ago

@ctrandrsn and I ran into some confusion when trying to use this feature. Specifically,

nilsvu commented 3 weeks ago

Sure I can do some of these. A few notes:

  1. Zero H5 files: Click has the explicit recommendation to gracefully degrade into noops if the input is empty: https://click.palletsprojects.com/en/8.1.x/arguments/#variadic-arguments. The recommendation reads like this:

If you come from argparse, you might be missing support for setting nargs to + to indicate that at least one argument is required.

This is supported by setting required=True. However, this should not be used if you can avoid it as we believe scripts should gracefully degrade into becoming noops if a variadic argument is empty. The reason for this is that very often, scripts are invoked with wildcard inputs from the command line and they should not error out if the wildcard is empty.

I just followed this recommendation and find it reasonable, but also got confused sometimes when the number of arguments was empty, so I don't have a strong opinion either way.

  1. The X Y vs X,Y should be made consistent, yes. It currently isn't because sometimes the dimension is fixed to 2 or 3, so nargs is used which parses as X Y, and sometimes the dimension is dynamic so we can't use nargs but have to parse the argument as a comma-separated list X,Y and check the dimension later. I haven't decided what to do about this yet (probably just always do X,Y).
nilsdeppe commented 3 weeks ago

Thanks!

  1. Hmm, yea, though I would expect here that not getting output is more confusing than getting an error. I personally disagree with the recommendation being as strong as it is. I think it's true for general processing scripts where it's perfectly reasonable to have nothing to process and that it should be fine without files. I think for scripts designed to only be used interactively and not as part of some large automated framework, succeeding without output is more confusing than helpful :shrug:. So, for example, having some cron job that tries to process files fail because there are none is bad. Having a script run by a user where the user clearly expects output fail when there is no output is good.
  2. Ah okay, that makes sense. I think this is not a big deal because it is actually documented. It was just surprising :)
nilsvu commented 3 weeks ago

Yea I agree with 1. Let's change it.

wthrowe commented 3 weeks ago

The reason for this is that very often, scripts are invoked with wildcard inputs from the command line and they should not error out if the wildcard is empty.

That's not how (Bourne shell) wildcards work. They can't be empty.

$ mkdir foo
$ cd foo
$ rm *.c
rm: cannot remove '*.c': No such file or directory

In bash there's an option for it, but I'm struggling to think of a command that actually becomes a noop in that case.

$ shopt -s nullglob
$ rm *.c
rm: missing operand
Try 'rm --help' for more information.