Ignore masked values for determining canvas and colorbar ranges #292
Conversation
|
Yeah this looks like a better solution. The autoscaling logic doesn't leak into the |
|
About the padding breaking the tests, perhaps we should make the padding width a global config and then import it both in |
We used to have a global configuration file which was mostly used for the plotting. I thought it would be useful for users to be able to set their prefered colormap, or their line-style. But it led to some annoyances in the code. When we migrated to Plopp, we left the config file out. No one has really complained that it was removed/missing. I agree that users may not want to set the padding or the line color as a kwarg every time they make a plot, which is why I thought of having such a config file initially. However, it seems that most people are just happy with the default Plopp/Matplotlib gives you, and that if they really need to control every aspect of the figure (for a paper for example), then they typically just set parameters for that one figure. It's not so bothersome. So I would say, for now, we don't do the config file. But if people start asking for it, we will re-instate it. |
src/plopp/core/limits.py
Outdated
| Return the union of this bounding box with another one. | ||
| """ | ||
| return BoundingBox( | ||
| xmin=min(self.xmin or np.inf, other.xmin or np.inf), |
There was a problem hiding this comment.
Wouldn't this break if self.xmin == 0? I think we need to explicitly check for None here even if it is more verbose. Or perhaps we can set the defaults already in the constructor?
| self._xmax = np.NINF | ||
| self._ymin = np.inf | ||
| self._ymax = np.NINF | ||
| self._bbox = BoundingBox() |
There was a problem hiding this comment.
Would it be possible to have this only be a local variable in the autoscale() method? I saw that it is used in logy but I didn't really understand why.
There was a problem hiding this comment.
It's also used in autoscale itself. It's used as a global memory of the range. It is needed when you are in a mode where you allow the plot limits to grow but not shrink.
The default is that the range follow the data, but you can also set a mode where limits grow, maybe giving a better feel for the relative differences in the data.
See the autoscale parameter in https://scipp.github.io/plopp/reference/generated/plopp.plot.html#plopp.plot
|
I now realise I also need to apply this to the plotly backend. |
src/plopp/core/limits.py
Outdated
| """ | ||
| Find sensible limits, depending on linear or log scale. | ||
| If there are no finite values in the array, raise an error. | ||
| If there are no positive values in the array, and the scale is log, fall back to | ||
| some sensible default values. | ||
| """ | ||
| if scale is None: |
There was a problem hiding this comment.
Minor: Since we are not doing anything special with the scale=None case it seems to me that it would be nicer to leave scale as linear/log here and let the caller worry about the None case.
| from scipp import Variable, scalar | ||
| import scipp as sc | ||
|
|
||
| from .utils import merge_masks | ||
|
|
||
|
|
||
| def find_limits( |
There was a problem hiding this comment.
Is find_limits called anywhere with a Variable valued argument for x?
There was a problem hiding this comment.
It shouldn't be the case anymore, because we are querying the .masks property of x.
Fixes #266
As always, it turned out more complicated than expected. I needed to handle a bunch of different cases with coordinates that contain strings or datetimes.
For the latter, logscale on a datetime axis currently does not crash but no padding is applied.
It is an edge case anyway, and renders funny in matplotlib. I don't think it is very useful as a plot.
At least it doesn't crash so users can anyway set their own limits.