Axis range play #59
Axis range play #59
Conversation
|
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.
|
|
@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:
|
…entences about existing options. Code changes: Prevent axis flipping by checking return from auto_range Cruft around value parsing in auto_range to give user friending messaging Added more verbose --help messaging for the new options
|
Update and CI running; have a 1-year old's birthday that I apparently can't miss, so will check on this later. :) |
| @@ -72,6 +97,14 @@ def main(): | |||
| help = 'Show the subtracted sky flux instead of the object flux.') | |||
| parser.add_argument('--add-sky', action = 'store_true', | |||
| help = 'Add the subtracted sky to the object flux (overrides show-sky).') | |||
| parser.add_argument('--flux-range', type=str, default='0.5%:99.5%', metavar="MIN:MAX", | |||
There was a problem hiding this comment.
I appreciate that this matches the original behavior but perhaps '0.0:99.5%' is a better default for flux?
There was a problem hiding this comment.
There seem to be fewer deep troughs that big spikes, but the extremes of wavelength usually have noise that goes much lower than the rest of the minimums; probably worth chopping that off, keeping the 0.5% minimum.
|
I just noticed that |
|
The |
* Moved bossplot mask code around so it uses left_axis for plotting, height; sets this only after height set by any options
|
Should be fixed now |
|
The following
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 |
|
Okay, updates away:
The negatives are a pain. The following also works: |
|
Adding a space works too: |
|
These are creative solutions, but not very robust or intuitive. Is there a problem with using |
…rpretting negative values.
|
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 |
|
I'm happy to have it die (gracefully) on error. I was calling out 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 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. |
…xiting. Put back in the limit checking, which got dropped somewhere (making sure lower limit is actually lower than upper limit)
|
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_layoutby 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.