Closed dmargala closed 9 years ago
Hi @dmargala , quick-ish look:
Your code definitely looks smoother, and looks like we arrived at some similar decisions (e.g. treating wlen/x-axis as percentage, not percentile) and the similar options handling (e.g. mix'n'match pcts and numbers)
Three differences I do see:
I think the figsize option is useful (although its really a separate feature).
Percentiles should be based only on visible and valid values.
No flipped axes, please :-) What's next, emoticons?
@dcunning11235, I've included a filter for the wavelength range in the latest commit. The flipped axis feature is just how matplotlib works, I didn't really do anything special. I discovered the "feature" while testing for failure modes. I think it's harmless to leave as is.
$ bossplot --plate 4203 --mjd 55447 --fiber 879 --figsize 12,4 --show-dispersion --exposure 1 --cframe --flux-range 1%:99% --wlen-range 4000:5000
$ bossplot --plate 4203 --mjd 55447 --fiber 879 --figsize 12,4 --show-dispersion --exposure 1 --cframe --flux-range 1%:99% --wlen-range 4000:4800
@dcunning11235 @dmargala Can you both agree on one pull request, taking the best of both worlds, and then assign to me for the final code review.
@dmargala Are you all done? I'm going to add scripts.rst changes into your branch and look for any tweaks to add; so far thinking:
Update and CI running; have a 1-year old's birthday that I apparently can't miss, so will check on this later. :)
I just noticed that --show-dispersion
and --show-mask
don't work together although they both work separately. Assuming its a simple fix, could you add it to this PR?
The --show-dispersion
& --show-mask
issue is fairly straight-forward, fixing
Should be fixed now
The following --flux-range
specs do not do what I would expect:
range | expected | actual outcome |
---|---|---|
0:0 | error message | small range centered on zero with matplotlib UserWarning |
inf:inf | error message | ditto |
nan:nan | error message | same small range with no warning (!) |
'-1:1' | range [-1:+1] | bossplot: error: argument --flux-range: expected one argument |
'-1:1' | range [-1:+1] | Warning: invalid limit specified (\-1:1) using default. (but default is -20??) |
0.5%: | same as default | Not the same as 0.5%:99.5% |
:99.5% | same as default | ditto |
:200% | error message | Warning: invalid limit specified (:200%) using default. (but then it doesn't use default) |
Did I mention that this is a can of worms? :-)
In cases where you try to fall back to a default when an invalid limit is detected I would just exit with an error message: this is simpler to code and avoids the danger that the user misses the warning message and gets confused about how the options work.
The problem with an argument starting with a minus sign is tricky since argparse
insists on treating this like a new option. When I have run into this before, I ended up enclosing the range arg in [ ] so that any minus sign is not the first character. That would work here, unless you have a better idea.
Okay, updates away:
range | New behavior |
---|---|
min:max such that abs(max-min) < 0.1 | Range warning, use range default |
any nan or inf | Range warning, uses range default |
'' in --flux-range | Now default to proper percentile, not min/max of data range |
Errors in --flux-range | (ditto from '' case) |
The negatives are a pain. The following also works:
bossplot --flux-range="-10:100"
I ran into this in the first iteration on this, and there doesn't seem to be a particularly better way to solve.
Adding a space works too:
--flux-range " -10:100"
These are creative solutions, but not very robust or intuitive. Is there a problem with using [min:max]
?
Added '[]' argument wrapping
As far as messaging vs. failing goes, Daniel and I discussed this a bit yesterday. Of particular concern are --no-display cases.
Might it make sense to have yet another option like "--no-fail" that specifies the current warning message approach, and default behavior that simply fails on errors? Or does that only further complicate things?
There are some cases that call for extraordinary measures to keep going in the face of invalid args, like saving the output of a week-long calculation when the filename arg has a typo. I don't think this is one of those cases, so simple and maintainable code should be the priority.
I don't see a problem with --no-display
exiting with an error. What scenario do you have in mind?
I'm happy to have it die (gracefully) on error. I was calling out --no-display
as a particular case where it would be good to die: no one is (immediately) looking at the output and they may not notice the message.
But let me try to (partially) argue for this anyway:
Handling defaults doesn't particularly complicate the code because we're already allowing for partial ranges like :50%
. And :
, for that matter. Returning defaults on parse errors is a minor addition, and the handling of 'trick' values like 'inf' is a couple of additional lines on top of that.
That said, I completely agree that usability-wise it is better to fail. Daniel jump in here, or else I'll change the current messaging to messaging + exit.
I am looking at this now...
I made minor some adjustments:
Here is another implementation of axis limit options for bossplot (see #54 #27).
I added an argument to specify the figure size (and use
tight_layout
by default):Absolute and quantile limits can be mixed, matched, and partially omitted:
Even upside down and backwards:
I considered trying to unify the x-axis and y-axis methods but decided that there is a fundamental difference in how quantiles should be interpreted for them.