Skip to content
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

ENH: Add time_format to Raw.plot() #9419

Merged
merged 51 commits into from
Jun 9, 2021
Merged

Conversation

marsipu
Copy link
Member

@marsipu marsipu commented May 21, 2021

Reference issue

Tackles #9335 (@cbrnr).

What does this implement/fix?

This adds a parameter (time_format) to Raw.plot() to display the real time derived from Info['meas_date'].

@marsipu marsipu changed the title ENH: Add show_real_time to Raw.plot() WIP: Add show_real_time to Raw.plot() May 21, 2021
@marsipu
Copy link
Member Author

marsipu commented May 24, 2021

I tried many ways of escaping in RST, but none allowed me to include "%" from the datetime format code.
Do you know another way?

@marsipu
Copy link
Member Author

marsipu commented May 24, 2021

When I try to build the documentation with make html_dev-noplot, I get:

Handler <function make_redirects at 0x0000027ECEBCDEE0> for event 'build-finished' threw an exception (exception: 'auto_tutorials' is not in list)

I assume it is not related to the changes from this PR, right?. I already updated my sphinx-installation from requirements_doc.txt.

@agramfort
Copy link
Member

your doc CIs are green. Do you have this pb locally?

mne/viz/ica.py Outdated Show resolved Hide resolved
@marsipu
Copy link
Member Author

marsipu commented May 24, 2021

your doc CIs are green. Do you have this pb locally?

Yes, its locally (on Windows). I reinstalled my mnedev-environment, but the error persists.

@marsipu
Copy link
Member Author

marsipu commented May 24, 2021

And locally I can't replicate the error in test_epochs::test_metadata from the CIs:

mne\tests\test_epochs.py:2766: in test_metadata
    meta = np.array([[1.] * 5 + [3.] * 5,
E   FutureWarning: Promotion of numbers and bools to strings is deprecated. In the future, code such as `np.concatenate((['string'], [0]))` will raise an error, while `np.asarray(['string', 0])` will return an array with `dtype=object`.  To avoid the warning while retaining a string result use `dtype='U'` (or 'S').  To get an array of Python objects use `dtype=object`. (Warning added in NumPy 1.21)

@larsoner
Copy link
Member

I pushed a fix for that in #9422, feel free to ignore it. It's due to using latest NumPy where they've deprecated something

@marsipu marsipu changed the title WIP: Add show_real_time to Raw.plot() ENH: Add show_real_time to Raw.plot() May 24, 2021
mne/io/base.py Outdated Show resolved Hide resolved
doc/changes/latest.inc Outdated Show resolved Hide resolved
mne/viz/_figure.py Outdated Show resolved Hide resolved
mne/viz/_figure.py Outdated Show resolved Hide resolved
mne/viz/_figure.py Outdated Show resolved Hide resolved
mne/viz/_figure.py Outdated Show resolved Hide resolved
mne/viz/ica.py Outdated Show resolved Hide resolved
mne/viz/ica.py Outdated Show resolved Hide resolved
@cbrnr
Copy link
Contributor

cbrnr commented May 25, 2021

I have two additional suggestions that I'd like to discuss:

  1. Can we make this setting toggle-able? It would be nice if we could just switch back and forth between the two time formats.
  2. When zoomed in far enough, identical time stamps might show up multiple times because the lowest resolution seems to be seconds (see screenshot). I think there should be fractions of a second in such cases.

Screen Shot 2021-05-25 at 11 34 47

WDYT?

Does anyone have a suggestion for a better parameter name instead of show_real_time? @larsoner @agramfort or @drammock maybe?

Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested on some over night sleep EEG data and it worked great.

I am thinking we should be able to toggle the time format with eg the 't' shortcut. Doing so I am less worried about parameter API so I will expect users to toggle from the GUI the option.

my 2c

doc/changes/latest.inc Outdated Show resolved Hide resolved
mne/viz/_figure.py Outdated Show resolved Hide resolved
@marsipu
Copy link
Member Author

marsipu commented May 25, 2021

@agramfort, @cbrnr Thank you for the comments&ideas, I will look into the toggle-behaviour and the zoom-dependent format-changes.

@drammock
Copy link
Member

For the API, what about time_format = 'float' | 'datetime' ?

@agramfort
Copy link
Member

agramfort commented May 25, 2021 via email

@marsipu
Copy link
Member Author

marsipu commented May 26, 2021

If you view the plot with a "usual" duration-window, milliseconds aren't probably that important. By adding up first_time and meas_date considering milliseconds (and even microseconds for larger Zoom-Levels), these will introduce an offset in milliseconds which will be visible at every tick even at smaller Zoom-Levels. A solution could be subtracting this tiny offset to make the ticks look cleaner at smaller Zoom-Levels. But this would also mean to loose +-0.5s of accuracy on the plot (relative to the "real" time).
WDYT?

@cbrnr
Copy link
Contributor

cbrnr commented May 26, 2021

I don't quite understand - can you show a figure that demonstrates the problem (and also your suggested solution/workaround)?

@marsipu
Copy link
Member Author

marsipu commented May 26, 2021

Now, the X-Axis for a plot with duration=10 of the sample-dataset looks like this:
grafik

With my workaround, this Zoom-Level would look like this:
grafik

And a higher Zoom-Level would look like this:
grafik

So we would round the time to seconds for visibility. I would implement it like this:

            import datetime
            first_time = self.mne.inst.first_time
            meas_date = self.mne.inst.info['meas_date']
            seconds = int(xval + first_time)
            milliseconds = int(xval % 1 * 1e3)
            # Add rounded offset from first_time and meas_date to improve
            # readability on smaller zoom-levels.
            # Introduces inaccuracy of max. ±0.5 s from real time.
            seconds += round(first_time % 1 + meas_date.timestamp() % 1)
            # Round meas_date to seconds.
            meas_date = meas_date.replace(microsecond=0)
            # Determining datetime with accuracy of microseconds to add
            # up on first_time and meas_date correctly.
            xdatetime = meas_date + datetime.timedelta(seconds=seconds,
                                                       milliseconds=
                                                       milliseconds)
            if milliseconds != 0:
                xdtstr = xdatetime.strftime('%H:%M:%S.%f')[:-3]
            else:
                xdtstr = xdatetime.strftime('%H:%M:%S')

@drammock
Copy link
Member

@marsipu at the higher level zoom the times are out of order (55, 56.5, 56, 57.5, 57) in your screenshot

@marsipu
Copy link
Member Author

marsipu commented May 27, 2021

@marsipu at the higher level zoom the times are out of order (55, 56.5, 56, 57.5, 57) in your screenshot

Ah thanks, I overlooked it. The problem was:

seconds = int(xval + first_time)

which also added up milliseconds. I changed it to:

seconds = int(xval) + int(first_time)

What do you think about introducing this small artificial offset in favor of visualization? I also added the offset to the x-axis-label now so it would be transparent:
grafik

@marsipu marsipu requested a review from cbrnr May 27, 2021 08:38
@cbrnr
Copy link
Contributor

cbrnr commented May 27, 2021

I would only show fractions of a second when necessary (i.e. when zoomed in enough). So even if meas_date is something like 19:01:53.676 I wouldn't show it when viewing 10s segments of data, but only in cases where these fractional seconds are significant. Furthermore, I'd never show trailing zeros (e.g. 19:01:54.5 instead of 19:01:54.500) and I wouldn't show the offset. My initial concern was not that we don't represent time exactly (it's OK to round), but when zoomed in it is very confusing if all time stamps are identical because of rounding. In that case, I'd show more exact time stamps.

@marsipu
Copy link
Member Author

marsipu commented May 27, 2021

To summarize:

  • Remove the display of the offset
  • Avoid trailing zeros

We could also display the exact time (not rounded) only when zooming in. The only thing that could be confusing for the user would be then, that 19:01:53 would then be e.g. 19:01:53.676 at higher zoom-levels.
Would you prefer the rounding solution or the exact solution?

@larsoner
Copy link
Member

Could we programmatically change the sub-second rounding level based on duration shown (zoom)? For example something like this, showing how np.pi seconds could be formatted, seems reasonable:

>>> for duration in (0.09, 0.2, 1.5, 8., 20., 101.):
...     fmt = '{t:0.%df}' % max(int(np.ceil(-np.log10(duration))) + 2, 0)
...     print(f'{duration}: ' + fmt.format(t=np.pi))
... 
0.09: 3.1416
0.2: 3.142
1.5: 3.14
8.0: 3.14
20.0: 3.1
101.0: 3

@cbrnr
Copy link
Contributor

cbrnr commented Jun 7, 2021

@drammock I don't think there was anything wrong conceptually with the current zooming approach, only the last zoom level seems to be broken. So I wouldn't change it and instead fix the last level.

@drammock
Copy link
Member

drammock commented Jun 7, 2021

@drammock I don't think there was anything wrong conceptually with the current zooming approach, only the last zoom level seems to be broken. So I wouldn't change it and instead fix the last level.

The current approach is "always change by one second, unless that would be less than 3 samples, in which case show 3 samples (because anything less than 3 samples is unintelligible)". I don't consider that "broken" (@marsipu has already made it so that going back from 3 samples to 1 second works properly --- i.e., it no longer goes from 3 samples to 1 second + 3 samples), but I can agree that it might sometimes be useful to have access to zoom levels less than 1 second but more than 3 samples. That is what I thought you wanted, which is why I implemented the pseudo-logarithmic approach. Did you actually try it out, and find it worse than the 1-second-at-a-time zooming? If so, then what other approach do you propose?

@cbrnr
Copy link
Contributor

cbrnr commented Jun 7, 2021

So in short currently it is not possible to zoom in less than one second because it immediately goes to three samples? I haven't tried your suggestion because I wanted to understand the current behavior, so I will now try it out! It would probably make sense to always zoom by a factor of two, let's see.

@marsipu
Copy link
Member Author

marsipu commented Jun 7, 2021

So in short currently it is not possible to zoom in less than one second because it immediately goes to three samples?

Yes, this was true before @drammock's solution c524586. I like his new approach, because it provides small zoom-steps, keeps equal steps in both directions and also makes zooming-out faster which I would appreciate as a user.

@cbrnr
Copy link
Contributor

cbrnr commented Jun 8, 2021

@drammock @marsipu I tested the new behavior and it is much better than the old one (I actually think the old one was broken because not being able to zoom in less than a second should have resulted in not zooming in any further instead of showing 3 samples). If we wanted to do some bikeshedding, we could try the factor 2 approach, but I'm also fine with what you already implemented.

@marsipu regarding the clock labels, I think this is ready now. Very nice work, this is a badly needed feature!

@marsipu
Copy link
Member Author

marsipu commented Jun 8, 2021

Thank you all for your support and improvements.
I think the current test failures are unrelated (something pyvista related, test_brain.py test_renderer.py).

@drammock
Copy link
Member

drammock commented Jun 8, 2021

OK, I've done one final pass over the code and played around some more with the feature interactively. I'm quite happy with how this turned out @marsipu! I added a couple more code comments, and tweaked the behavior very slightly:

  1. always shows millisecond-precision in the green vline text (prev. sometimes showed only 2 decimals if zoomed out very far)
  2. still truncate to integers when possible (if int(x) == x) but do not force this to always happen on the hscroll axis (i.e., even when int(x) != x). Most of the time matplotlib will choose clean integer-valued ticks anyway, and better to be accurate in case someone loads a very short Raw file where the hscroll axis only spans a couple seconds.

+1 for merge when CIs are finished.

@cbrnr
Copy link
Contributor

cbrnr commented Jun 9, 2021

Works great! I have one final (minor) nitpick, something which has been bothering me for a long time, and maybe this can be fixed here: there is no tick mark to the very right of the visualized time period. For example, the default duration is 10s, so I get labels 0, 2, 4, 6, 8 - but not 10. I'd find it super useful if the plot also showed the last time stamp (for both float and clock formats). Is this possible?

@marsipu
Copy link
Member Author

marsipu commented Jun 9, 2021

Is this possible?

The problem as I see it is, that xlim of ax_main is set to the last time-point of times (l. 2185 of _figure.py).
Currently for duration = 10s, this last time-point is 0.999 (for sfreq = 1000, l.1954 of _figure.py). If that is not on purpose this Off-by-one-error would be one reason.
Another reason could be division-inaccuracies when sfreq != 1000. (with the off-by-one-fix for example I still see the problem for the sample-data with sfreq = 600.614...). I tried just rounding the upper xlim with np.ceil but the problem is, that we then couldn't accept float-values for duration anymore, because we would than either have whitespace in the plot or clip a part of it. Would that be a justifiable change?

@marsipu marsipu changed the title ENH: Add show_real_time to Raw.plot() ENH: Add time_format to Raw.plot() Jun 9, 2021
@cbrnr
Copy link
Contributor

cbrnr commented Jun 9, 2021

Nice! I think it's really the range - with this change the last tick is now included!

@marsipu
Copy link
Member Author

marsipu commented Jun 9, 2021

Nice! I think it's really the range - with this change the last tick is now included!

@cbrnr But not for the sample-data, right?

@cbrnr
Copy link
Contributor

cbrnr commented Jun 9, 2021

Which sample data do you mean? I tried with n1.edf from the URL I provided previously, and it works.

@marsipu
Copy link
Member Author

marsipu commented Jun 9, 2021

Which sample data do you mean? I tried with n1.edf from the URL I provided previously, and it works.

I meant the dataset from mne.datasets.sample.data_path()

@cbrnr
Copy link
Contributor

cbrnr commented Jun 9, 2021

Any particular file from this data set (can you provide the code)?

@marsipu
Copy link
Member Author

marsipu commented Jun 9, 2021

import os
import mne
sample_data_folder = mne.datasets.sample.data_path()
sample_data_raw_file = os.path.join(sample_data_folder, 'MEG', 'sample',
                                    'sample_audvis_raw.fif')
raw = mne.io.read_raw(sample_data_raw_file)
raw.plot(block=True, duration=10)

@cbrnr
Copy link
Contributor

cbrnr commented Jun 9, 2021

I see. That's a bit unfortunate, I'll think about it. But for some data sets at least it seems to improve things at least.

@drammock
Copy link
Member

drammock commented Jun 9, 2021

I would try increasing the user-requested duration by one or two sampling periods, rather than using np.ceil

@marsipu
Copy link
Member Author

marsipu commented Jun 9, 2021

@drammock you mean just like that? Yeah, nice idea, this is so small that you wouldn't notice on the plot

@cbrnr
Copy link
Contributor

cbrnr commented Jun 9, 2021

Good idea, now it also works for the sample data! 🥳

@drammock drammock merged commit bd64455 into mne-tools:main Jun 9, 2021
@drammock
Copy link
Member

drammock commented Jun 9, 2021

in it goes! Thanks @marsipu and @cbrnr!

@marsipu marsipu deleted the x_time_ticks branch June 9, 2021 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants