| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
There was a problem hiding this comment.
Please keep within 80 characters line width as per PEP8.
Sorry, something went wrong.
There was a problem hiding this comment.
Shouldn't this be log_base ** -1? (i.e. 0.1 only makes sense when the log base is 10).
Sorry, something went wrong.
There was a problem hiding this comment.
Yes, but how do I access the base of log scale axis?
Sorry, something went wrong.
There was a problem hiding this comment.
Ah, I see. I missed the fact that hist doesn't take a base as argument. hist is a bit of an anomaly -- it takes a 'log' argument, whereas most other things (except bar*) work by requiring a subsequent call to set_xscale or set_yscale, which of course take an optional "base". Making this also work that way would be a little bit of work, because this minimum adjustment stuff you've added here would have to be moved to the draw phase. Ideally, that's where I'd like to see this go, but I don't know if that's worth abandoning this fix, and we've have to maintain backward compatiblity anyway -- this "log" kwarg goes back at least 5 years, as far as I can tell.
We could either add an additional keyword "log_base", or extend "log" so it can be "True" (meaning base 10 as it does now), or a number, meaning a base. I would also recommend updating bar* to be in line with that.
However, I'm aware that none of this is premised on any real world use case -- just my own desire for consistency across uses of log scale, so I could be persuaded to leave this as-is by further discussion.
Sorry, something went wrong.
There was a problem hiding this comment.
This might be kind of hard to set the minimum correctly if log base can be changed between to calls. Consider this
ax.hist( x1, log=10)
ax.hist(x2, log=2)The first hist call would have no idea that eventually the base of log scale will be changed to 2.
But, I do not think it would be hard for use to do another call to set_ylim though. The histogram will be painted all the way down (see line 7892) anyway.
Sorry, something went wrong.
There was a problem hiding this comment.
Yes -- that, in a nutshell, is why I'm not a fan of "log" being a kwarg to this method. You may want to hold of on implementing any sort of fix for this issue until we discuss it -- it may be best to just leave as-is.
@efiring, @WeatherGod, @pelson: any thoughts?
Sorry, something went wrong.
There was a problem hiding this comment.
w.r.t. not being able to access the axes's log base, we are assuming base 10 throughout this part of the code, and we have always done so before. We could always look to future PRs to address the handling of arbitrary log bases. I see this more as a good bug fix to an outstanding issue.
I guess it would be smart, though, for future developers, to somehow comment the spots that are assuming base 10 so that they know to change it if/when we extend the interface in the future to accept arbitary log bases.
Sorry, something went wrong.
There was a problem hiding this comment.
I tend to agree with @WeatherGod here: let's leave this assuming base 10 for now.
Sorry, something went wrong.
|
log in hist has always been an odd duck. It tries to be like the bar* functions, but not quite. Note, however, that unlike the bar* functions, a log kwarg in hist actually has a purpose besides convenience. While one could pre-transform their data for the bar* functions, one can't with hist because what is plotted is the result of an internal aggregation calculation. There is another PR that I am waiting for a few last polishings that also addresses an issue for log scaling of the results for hist2d(). Might be a good idea to make sure both functions support the same "log" kwarg inputs. |
Sorry, something went wrong.
There was a problem hiding this comment.
Wrap these comment lines to less than 75 characters
Sorry, something went wrong.
There was a problem hiding this comment.
75? I thought we did 80?
Sorry, something went wrong.
There was a problem hiding this comment.
Actually, I was wrong, in PEP8, docstrings are recommended to be at most 72 characters.
Sorry, something went wrong.
|
Great job with the additional tests. Only one request here about them. Think we could have corresponding tests for the horizontal case? |
Sorry, something went wrong.
There was a problem hiding this comment.
My comments above wrt multiple calls to hist() goes here.
Sorry, something went wrong.
There was a problem hiding this comment.
This is kind of the part I don't fully understand. What function does 0.9 serve and where should I get the old bound. What's the difference between self.datalim.bound and self.datalim.intervalx/y?
Sorry, something went wrong.
There was a problem hiding this comment.
I don't have time for a full explanation as I am about to head off on
vacation, but perhaps I can provide some clues for you. I think this code
part has been neglected. Other functions like plot() and scatter() utilize
pads for the data limits so that the axes wouldn't be extremely tight
around the data being plotted. This value used to be a hard-coded 0.05 on
each endside (for a total of 0.1). In plot() and scatter() (and possibly
bar()) we now handle this a lot more intelligently, but I think hist() was
left behind in this regard.
Note that data limits don't necessarially mean the same as view limits. I
am not sure what the distinction is between data limits and data interval
is, though.
I hope this helps!
Ben Root
Sorry, something went wrong.
|
Does this pull request need any more work? histtype='stepfilled' also produces pretty unsightly results on master. |
Sorry, something went wrong.
|
I think it needs testing on multiple plots being drawn together. I'm not 100% sure what _save_bounds does. |
Sorry, something went wrong.
|
They seems to fix the same thing. I'll close this. |
Sorry, something went wrong.
|
@piti118 Judging by the title of this PR, this patch seems to address behaviour that can be seen when using a linear scale rather than a log scale. PR #1684 only addresses an issue with log scaling. Therefore it might be the case that this may need to be reopened. Thanks for taking the time and the effort to write a patch for the codebase, @piti118. Again, if you see unexpected behaviour with linear scaling then please feel free to re-open this. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Fix two closely related issue:
#1066
and
#196