-
Notifications
You must be signed in to change notification settings - Fork 72
Implement soft delay extension #264
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
base: master
Are you sure you want to change the base?
Conversation
0abc0cf
to
240e9a7
Compare
Regarding the failing sequence tests, you can set the environment variable SAVE_EXPECTED=1 and run pytest to locally rewrite the .seq files. This will automatically pass the write tests, but then allows you to do a diff with the previous .seq files (e.g. git diff, or make a copy of the .seq files first). Then if you manually verify that the change of IDs is expected, commit the new .seq files. I know manually checking this is a bit of a pain, but I think aiming for a exact match of the .seq files is a good standard, better safe than sorry. |
I think this is ready for testing, but there are several things that may require polishing.
|
based on the user input (to the interpreter) according to the equation dur=input/factor+offset. | ||
Required parameters are 'numeric ID' and 'string hint'. Optional parameter 'factor' can be either | ||
positive and negative. Optional parameter 'offset' given in seconds can also be either positive and | ||
negative. The 'hint' parameter is expected to be for identical 'numID'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'hint' parameter is expected to be identical (?) for identical 'numID'.
src/pypulseq/Sequence/sequence.py
Outdated
from pypulseq.supported_labels_rf_use import get_supported_labels | ||
from pypulseq.utils.cumsum import cumsum | ||
from pypulseq.utils.tracing import format_trace, trace, trace_enabled | ||
|
||
major, minor, revision = __version__.split('.')[:3] | ||
|
||
|
||
class BiDict: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this really necessary?
do we need lookup by "id" and by "hint"==name?
cant we just assume that insertion order is equal to id?
or is there an actual use case where one wants to specify the soft delay id when creating it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems tedious to me to have both id and hint, which according to the documentation for make_soft_delay
needs a one-to-one relation anyway. I don't see why we can't just refer to a delay always by name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quoting myself from another feedback:
I think the main purpose of the numID is letting the user choose which delay box they want to use for a specific delay. For example, if they want to put TE first then TR, they can do so by explicitly assigning IDs 0 and 1, to them, regardless of the order they are added into the sequence.
But, I agree with you. It creates confusion for little gain. Also, I tried setting numID to 2 and did not assign anything to 0 and 1, and interpreter shows 3 delays anyway since it has to create them sequentially. It gracefully handled empty events, but still.
I think we can deviate from the Matlab here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a quick look -- and noticed i need more time too understand the logic and play around with the code....
regarding check timings: for it to make sense, it would need to get the information about the values that should be used during the check, correct? maybe a dict of values? or would the expected workflow be save seq, apply_soft_delay for all delays, check_timings ? |
PS: I am stricly in favor of diverting from matlab to make parameters and functions more explicit and pythonic. |
10e-6, # Add a small delay as we can't have soft_delay in an 0 duration block. | ||
pp.make_soft_delay(numID=0, hint='TE', offset=-min_TE, factor=1.0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really dislike this as it is counter-intuitive (soft delay blocks must be empty, but then must have a duration...). I would prefer if make_soft_delay
has a default delay value (required to be >0
) that gets used as the block duration.
On a sidenote, I really think the float input as an option to add_block
should not exist in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. For now, I removed the required_duration
, now default_duration
is an input to make_soft_delay
with a default value of 10-e5.
On a sidenote, I really think the float input as an option to add_block should not exist in the first place.
I looked into this, but I felt like removing that will cause bunch of other issues to solve: 99bbffd
src/pypulseq/make_soft_delay.py
Outdated
|
||
Parameters | ||
---------- | ||
numID : int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the reason the numID
parameter exists and is not just assigned automatically, as is done for other events. If the hint
needs to be identical for identical numID
, then the delay can be uniquely addressed with just the hint
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the main purpose of the numID
is letting the user choose which delay box they want to use for a specific delay. For example, if they want to put TE first then TR, they can do so by explicitly assigning IDs 0 and 1, to them, regardless of the order they are added into the sequence.
But, I agree with you. It creates confusion for little gain. Also, I tried setting numID
to 2 and did not assign anything to 0 and 1, and interpreter shows 3 delays anyway since it has to create them sequentially. It gracefully handled empty events, but still.
I think we can deviate from the Matlab here.
if hint not in sd_str2numID: | ||
raise ValueError(f"Specified soft delay '{hint}' does not exist in the sequence") | ||
|
||
def get_default_soft_delay_values( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The functionality of get_default_soft_delay_values
does not match its name. Checking consistency of the events should be a single function, and that function should be called somewhere to ensure the sequence is error-free. Or add the checks to check_timing
(which is called by default from write_seq
).
Getting default values should be another, separate function.
General comment: It's probably a good idea to add |
@FrankZijlstra @fzimmermann89 Many thanks for the comments. It looks like #288 takes this PR and has many changes on top of this. I am not sure how to proceed with this without duplicating effort and creating some conflicts. It looks like there are some changes to soft delay part, but most changes on #288 are for other features. If @mcencini wants to adress these feedback on there I can close this PR. |
@btasdelen apologies, I did not mean to create confusion. The only changes in the soft-delay-part are some bug fixes and a simple test case that I wanted to add to this PR (see btasdelen#2). The idea was to develop the remaining v1.5.0 features mentioned in #258, but wait until this PR was merged. That said, I am happy to help polishing this. If you think you don't need to have soft delay extension in 1.4.x, I can address it in #288. The only issue is that the v1.5.x changes are quite a lot and I unfortunately I do not see an easy way to split it in multiple smaller PR for easy review, except for soft delay which is quite independent from the rest. Suggestions are welcome, either in #288 or #258 threads! :D |
@mcencini No worries. That is why I started with soft delays, it was pretty much independent of the rest of the stuff. I can try cherry-picking your relevant commits. If I make changes and merge this, I believe you will have to deal with some conflicts when you update your branch. I am fine either way. My opinion is, regardless who deals with the rest, it is better to divide things into smaller PRs, even if the individual PRs are not complete v1.5.0 compatible. |
@fzimmermann89 @FrankZijlstra I had a crack at making Another way to approach this is to make use of the Also, I did not remove |
@fzimmermann89 @FrankZijlstra I think this is the only thing left before the v1.5.0 release. Let me know if you have any further comments. I know it is not perfect, but one option to be just merge this and see if folks use it and have any bug reports on it. Since this is pretty much decoupled from the rest of the code, it should be okay. |
I think it would be okay to merge it with a few minor changes:
|
…or report from get_default_soft_delay_values
|
Work in progress. Effort to implement soft delay extension for file format 1.5.0 compatibility.
TODO:
soft_delay
handling toaddBlock()
.apply_soft_delay()
.soft_delay
handling towrite()
.soft_delay
handling toread()
.get_default_soft_delay_values()
check_timing()
.soft_delay
handling inseq.plot()
.register_soft_delay_event()
Challenges/Concerns:
.seq
files do not match. Invesigate/fix.checkTiming()
andseq.plot()
. What should we do?