Add BaseRaw.duration property and fix duration calculation for BaseRaw "repr/html" display

[leorochael]

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 used self.n_times / self.info['sfreq'] instead, as self.times is calculated from the same info self.n_times is calculated from, but self.n_times seems to be "cheaper".

I humbly suggest reviewing commit by commit.

[welcome[bot]]

Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴

[cbrnr]

Quick question, would this not also be relevant for the normal __repr__?

[leorochael]

Quick question, would this not also be relevant for the normal __repr__?

@cbrnr, good point! For a 500 hz EDF, the repr actually reports something like:

<RawEDF | filename.edf, 30 x 1800000 (3600.0 s), ~34 kB, data not loaded>

So I didn't realize it was also using the wrong self.times[-1] duration. I'll try to reproduce a failure in a test.

[cbrnr]

Isn't 3600.0 s correct?

[leorochael]

Isn't 3600.0 s correct?

It is! But only because:

>>> f"{3599.99609375:0.1f}"
'3600.0'

[cbrnr]

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 tmin=-0.25 and tmax=0.75, and assuming you have a sampling frequency of 100Hz, which duration would you expect? Currently, the epoch is 101 samples long (1.01s), because it includes time point 0.

[leorochael]

A single sample can be interpreted as the measured value sampled at a particular time instant, so technically it has no duration.

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...

There is a related issue when creating epochs. If you specify tmin=-0.25 and tmax=0.75, and assuming you have a sampling frequency of 100Hz, which duration would you expect? Currently, the epoch is 101 samples long, so 1.01s, because it includes time point 0.

That depends on whether tmax represent an open or closed end of the interval... In my experience, treating the ends of intervals as being open (i.e. not included), with the starts being closed, makes it easier to stitch together contiguous time intervals (collections of contiguous epochs?).

[cbrnr]

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).

[leorochael]

In order to avoid repeating the self.n_times / self.info["sfreq"] calculation, can I create a BaseRAW.duration property?

I suppose this would mean introducing a new feature, rather than being a bugfix. I could introduce a BaseRAW._duration property instead...

[leorochael]

Another question, the Towncrier action failed, but I don't understand why. I added a new doc/changes/devel as required...

[cbrnr]

I like these changes, but I'll let @drammock decide (1) if it's OK to add a BaseRAW.duration property and (2) whether or not the duration calculation should be in a private method or just in _repr_html_.

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]

Did the changes suggested, both to towncrier files and eliminating the timedelta from the calculation.

By moving the np.ceil() call earlier, this helped resolve some other nonsense time rendering ("00:01:60" rather than "00:02:00" when rounding up fractions of a second).

I also enhanced the repr/duration-string tests with parametrize to test different situations.

Still need to understand the CI failures that are happening...

[leorochael]

According to this, it seems the new duration property reference cannot be found, but I don't understand where else I should declare it...

[leorochael]

All checks have passed. Should I rebase?

[drammock]

All checks have passed. Should I rebase?

no need. GitHub will squash-merge anyway. Thanks @leorochael!

[welcome[bot]]

🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪