Add BaseRaw.duration property and fix duration calculation for BaseRaw "repr/html" display #12955
Conversation
leorochael
requested review from
drammock,
cbrnr,
agramfort,
mscheltienne and
dengemann
as code owners
|
Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴 |
|
Quick question, would this not also be relevant for the normal |
leorochael
force-pushed
the
leorochael-fix-duration
branch
from
ad4353e
to
04fa9c1
Compare
@cbrnr, good point! For a 500 hz EDF, the repr actually reports something like: So I didn't realize it was also using the wrong |
|
Isn't 3600.0 s correct? |
It is! But only because: >>> f"{3599.99609375:0.1f}"
'3600.0' |
|
OK, this makes sense. A single sample can be interpreted as the measured value sampled at a particular time instant, so technically it has no duration. But I agree that in general, it makes more sense to take the number of samples and divide it by the sampling frequency to get the duration in seconds 😄. There is a related issue when creating epochs. If you specify |
I get that logic, but when you take a collection of samples as a collection, each individual sample is representative of the interval, at least in the sense that, until the next "beat" of the sampling frequency, no other sample will be present...
That depends on whether |
|
Yes, I totally agree with you (and these are two separate issues, currently the epochs interval is inclusive on both ends, and we should probably add an option to exclude the end time). |
|
In order to avoid repeating the I suppose this would mean introducing a new feature, rather than being a bugfix. I could introduce a |
|
Another question, the Towncrier action failed, but I don't understand why. I added a new |
mne/io/base.py
Outdated
| def _get_duration_timedelta(self): | ||
| seconds = self.n_times / self.info["sfreq"] | ||
| return timedelta(seconds=seconds) | ||
|
|
||
| def _get_duration_string(self, duration): | ||
| # https://stackoverflow.com/a/10981895 | ||
| hours, remainder = divmod(duration.seconds, 3600) | ||
| minutes, seconds = divmod(remainder, 60) | ||
| seconds += duration.microseconds / 1e6 | ||
| seconds = np.ceil(seconds) # always take full seconds | ||
|
|
||
| return f"{int(hours):02d}:{int(minutes):02d}:{int(seconds):02d}" |
There was a problem hiding this comment.
I'm in favor of the change to using self.n_times / self.info["sfreq"] instead of self.times[-1]. But the rest of this seems a bit overcomplicated, I'm not sure why timedelta is even needed? Why not this, either inside _repr_html_ or as a single helper method?
duration = self.n_times / self.info["sfreq"]
hours, remainder = divmod(duration, 3600)
minutes, seconds = divmod(remainder, 60)
duration = f"{hours:02.0f}:{minutes:02.0f}:{np.ceil(seconds):02.0f}"There was a problem hiding this comment.
Hi @drammock, I moved the timedelta back into the ._get_duration_string() method.
On the other hand, I've factored out the self.n_times / self.info["sfreq"] calculation into a new .duration property.
The timedelta usage was originally added to the string representation to use in the rounding-up of the seconds amount, so indeed there is no sense in having a method just to return it.
On the other hand, having a separate ._get_duration_string method helps testing it in isolation from the ._repr_html_() method which is barely tested otherwise.
There was a problem hiding this comment.
And I kept the timedelta on the refactored method, because I didn't want to mess with the logic for calculating _repr_html_.
But if you want it removed, I'm ok with that.
leorochael
force-pushed
the
leorochael-fix-duration
branch
from
04fa9c1
to
ff5fd78
Compare
|
I like these changes, but I'll let @drammock decide (1) if it's OK to add a Also, I have no idea why Towncrier is failing, both changelog entries look correct to me. Maybe because there are two and you are a new contributor? Also something for @drammock or @larsoner. |
leorochael
force-pushed
the
leorochael-fix-duration
branch
from
ff5fd78
to
5303a85
Compare
doc/changes/devel/12955.feature.rst
Outdated
| @@ -0,0 +1 @@ | |||
| Add :meth:`mne.io.base.BaseRAW.duration` property to centralize duration calculation for :meth:`mne.io.base.BaseRAW.__repr__` and :meth:`mne.io.base.BaseRAW._repr_html_`, by :newcontrib:`Leonardo Rochael Almeida`. | |||
There was a problem hiding this comment.
This file should be named 12955.newfeature.rst which is why towncrier is complaining.
There was a problem hiding this comment.
Also this will fail because:
- the classname is
BaseRawnotBaseRAW durationis an:attr:not a:meth:- I think(?) that you can't cross-reference the methods
__repr__and_repr_html_(you can only cross-ref things that are indoc/python_reference.rst(or the various files indoc/api/*.rstthat get included into it). I suggest rewording this to just say "there's a new convenience propertyRaw.duration" and don't mention the repr stuff.
doc/changes/devel/12955.feature.rst
Outdated
| @@ -0,0 +1 @@ | |||
| Add :meth:`mne.io.base.BaseRAW.duration` property to centralize duration calculation for :meth:`mne.io.base.BaseRAW.__repr__` and :meth:`mne.io.base.BaseRAW._repr_html_`, by :newcontrib:`Leonardo Rochael Almeida`. | |||
There was a problem hiding this comment.
Also this will fail because:
- the classname is
BaseRawnotBaseRAW durationis an:attr:not a:meth:- I think(?) that you can't cross-reference the methods
__repr__and_repr_html_(you can only cross-ref things that are indoc/python_reference.rst(or the various files indoc/api/*.rstthat get included into it). I suggest rewording this to just say "there's a new convenience propertyRaw.duration" and don't mention the repr stuff.
leorochael
force-pushed
the
leorochael-fix-duration
branch
from
5303a85
to
58a9d38
Compare
The `|` in the regex meant that any string containing `<RawArray ` would match, so the data in the repr was not being checked.
leorochael
force-pushed
the
leorochael-fix-duration
branch
from
58a9d38
to
49f62d2
Compare
It represents the duration in seconds of the acquisition. Centralize duration calculation for the textual (__repr__) and html (_repr_html_, used by e.g. Jupyter) display of mne.io.BaseRAW instances using the new property. Extract the method to get the human string from the duration property for use by the html representation, so it's easier to test later.
The duration calculation, used in `BaseRAW._html_repr_()` and `BaseRAW.__repr__()`, was taking the timestamp of the last sample as the duration of the acquisition, but was not accounting for the length of the last sample. Also, added tests for the refactored `BaseRAW.duration` property and `BaseRAW._get_duration()` method, and used a sfreq value that revealed the discrepancy in the duration calculation in the `BaseRAW.__repr__()` method. Finally, simplified the duration string calculation for the html display by rounding up all the duration seconds, not just the remainder after hour and minute calculations, thereby avoiding "00:01:60" calculations, which should have been "00:02:00", when there are fractions of a second remaining. Fixes: mne-tools#12954
leorochael
force-pushed
the
leorochael-fix-duration
branch
from
49f62d2
to
1c0902a
Compare
leorochael
changed the title
leorochael
changed the title
|
Did the changes suggested, both to towncrier files and eliminating the timedelta from the calculation. By moving the I also enhanced the repr/duration-string tests with parametrize to test different situations. Still need to understand the CI failures that are happening... |
|
According to this, it seems the new |
There was a problem hiding this comment.
Will push a commit once local build confirms it should fix things then mark for merge-when-green since @drammock's comments have been addressed. Thanks in advance @leorochael !
leorochael
changed the title
leorochael
changed the title
|
All checks have passed. Should I rebase? |
no need. GitHub will squash-merge anyway. Thanks @leorochael! |
|
🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪 |
Reference issue (if any)
Fixes: #12954
What does this implement/fix?
Now the duration calculation takes into account the length of the last sample, by calculating the duration in seconds based on the number of samples divided by the sampling frequency.
Additional information
Instead of using
len(self.times) / self.info['sfreq']as suggested in the bugfix, I usedself.n_times / self.info['sfreq']instead, asself.timesis calculated from the same infoself.n_timesis calculated from, butself.n_timesseems to be "cheaper".I humbly suggest reviewing commit by commit.