imr-framework / pypulseq

Pulseq in Python
https://pypulseq.readthedocs.io
GNU Affero General Public License v3.0
111 stars 58 forks source link

Update confusing documentation for calc_duration.py #158

Closed gabuzi closed 7 months ago

gabuzi commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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