-
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix hist limit issue for step* for both linear and log scale #1075
Conversation
@@ -7946,6 +7946,22 @@ def hist(self, x, bins=10, range=None, normed=False, weights=None, | |||
x += 0.5*(bins[1]-bins[0]) | |||
|
|||
if log: | |||
#in the case of log scale there are three cases to consider for the y axis limit | |||
#1) has zero or negative bin but other bins may be 0<n<1 due to weighting. we want positive minimum*0.1 to be the minimum |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep within 80 characters line width as per PEP8.
|
||
if np.any(ndata>0): | ||
if np.all(ndata[ndata!=0]>=1): | ||
minimum = 0.1 # case 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be log_base ** -1
? (i.e. 0.1 only makes sense when the log base is 10).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but how do I access the base of log scale axis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to agree with @WeatherGod here: let's leave this assuming base 10 for now.
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. |
@@ -7946,7 +7946,29 @@ def hist(self, x, bins=10, range=None, normed=False, weights=None, | |||
x += 0.5*(bins[1]-bins[0]) | |||
|
|||
if log: | |||
y[0],y[-1] = minimum, minimum | |||
#in the case of log scale there are three cases |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrap these comment lines to less than 75 characters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
75? I thought we did 80?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I was wrong, in PEP8, docstrings are recommended to be at most 72 characters.
Great job with the additional tests. Only one request here about them. Think we could have corresponding tests for the horizontal case? |
@@ -7970,22 +7996,14 @@ def hist(self, x, bins=10, range=None, normed=False, weights=None, | |||
|
|||
# adopted from adjust_x/ylim part of the bar method | |||
if orientation == 'horizontal': | |||
xmin0 = max(_saved_bounds[0]*0.9, minimum) | |||
xmin = minimum |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comments above wrt multiple calls to hist() goes here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Does this pull request need any more work? |
I think it needs testing on multiple plots being drawn together. I'm not 100% sure what _save_bounds does. |
They seems to fix the same thing. I'll close this. |
@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. |
Fix two closely related issue:
#1066
and
#196