-
Notifications
You must be signed in to change notification settings - Fork 5
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 cursor hover when datetime axis is present in figure #311
Conversation
@jl-wynen would be super if you could have a quick look at this |
# Annoying chain of conversion but matplotlib has its own way of converting | ||
# dates to numbers (number of days since epoch), and num2date returns a python | ||
# datetime object, while scipp expects a numpy datetime64. | ||
return sc.scalar(np.datetime64(mdates.num2date(x).replace(tzinfo=None))).to( |
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.
We typically say that scipp datetimes are in UTC. But this looks like the result will be in whatever timezone matplotlib uses but without timezone info. I am assuming mpl uses the local timezone unless specified otherwise in the input. Is that correct?
Did you check that the cursor label is consistent with the axis labels regardless of timezone? And the same for zooming, selecting rectangles, etc.
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 think that matplotlib defaults to UTC:
All the Matplotlib date converters, locators and formatters are timezone aware. If no explicit timezone is provided,
rcParams["timezone"]
(default: 'UTC') is assumed,
in https://matplotlib.org/stable/api/dates_api.html#matplotlib-date-format.
In the examples I used, the first point read from the cursor corresponds to the first value in the array:
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.
Ok. And zooming and selecting points with mpl toolbox all work?
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.
@@ -132,29 +133,6 @@ def __init__( | |||
if need_grid: | |||
self._ax.grid(True) | |||
|
|||
# Cache slicing order for hover values |
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.
Is this related to datetimes?
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.
Kind of, it was the old way of getting the values for the fomatter. This is where the units for each axis were used from, but now, we instead just use the units that are stored in the canvas. The info was basically stored in both places and I think it makes more sense to just have it on the canvas.
We basically need to slice the image data to get the z value. We use label-based slicing, and for that we need the dim associated to each axis, and the unit. The canvas knows both these things, and the canvas now just sends a tuple (dim, sc.scalar(value, unit=unit)
to the format_coord
which is used as is for slicing.
Fixes #310