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

Update confusing documentation for calc_duration.py #158

Merged
merged 2 commits into from
Dec 4, 2023

Conversation

gabuzi
Copy link
Contributor

@gabuzi gabuzi commented Dec 1, 2023

This really confused me, and I got wrong results initially as I was thinking that this would sum up the durations of the provided events, while it's identifying the maximum duration.

I went ahead and wrote a bunch of tests for this, covering a wide variety of event combinations (that are most likely unrealistic if this is to be used to identify the longest event in a block). Nevertheless, they may be useful to double check the event creation functions using the duration args.

@btasdelen
Copy link
Collaborator

That certainly is clearer and more accurate. The way I think about it is calc_duration gives the block duration, if we are to create a block with the input events. I don't know if event duration implies for most people the delay+waveform duration, or just the waveform duration. Do you think adding a remark like "... maximum duration of its events (block duration)" makes it clearer or more confusing?

@gabuzi
Copy link
Contributor Author

gabuzi commented Dec 2, 2023

That certainly is clearer and more accurate. The way I think about it is calc_duration gives the block duration, if we are to create a block with the input events. I don't know if event duration implies for most people the delay+waveform duration, or just the waveform duration. Do you think adding a remark like "... maximum duration of its events (block duration)" makes it clearer or more confusing?

No, I definitely think more is better. I was just a bit in a hurry there.

I think just adding a sentence of what "duration" actually means is enough.


Calculate the duration of an event or block.
The duration of an event is the
time taken by the actual event itself plus its delay.
If multiple events are provided, the calculated duration is for a block
comprised of these events, which is given by the maximum duration of the events.

Although it is possible to provide events that can't be actually be combined into a block
(e.g. multiple events of the same type), the result is still the maximum duration of all events.

What do you think of that?

I think the complexity/confusion comes from the "double-duty" currently provided by this function ((1) calculate
actual event duration and (2) calculate the duration of these events as a block).
For future direction (object oriented refactor), I would suggest to implement a duration property on each event,
such that (1) vanishes.

@btasdelen
Copy link
Collaborator

Calculate the duration of an event or block.
The duration of an event is the
time taken by the actual event itself plus its delay.
If multiple events are provided, the calculated duration is for a block
comprised of these events, which is given by the maximum duration of the events.

Although it is possible to provide events that can't be actually be combined into a block
(e.g. multiple events of the same type), the result is still the maximum duration of all events.

I think this is pretty good. We can use this.

I also agree, with the class implementation, each event should be able to report their own duration without the need for calc_duration for case (1). This version of the doc clearly conveys case (2).

@btasdelen btasdelen merged commit 03db4c2 into imr-framework:dev Dec 4, 2023
4 checks passed
@schuenke schuenke mentioned this pull request Jun 6, 2024
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.

2 participants