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

Fixed set_block to give errors when multiple events of the same type are specified #141

Merged
merged 1 commit into from
Dec 13, 2023

Conversation

FrankZijlstra
Copy link
Collaborator

As title. There are two options here:

  1. Only first commit: Give errors for all event types
  2. Both commits: Give errors for RF/ADC events, but automatically add gradients together

Personally I'm in favour of option 2, because it seems like pretty natural behaviour, and it can come up in some instances: e.g. specifying a slab-selection refocusing gradient along with a phase-encoding gradient in 3D imaging. An argument against this behaviour is that it hides something the user should ideally be aware of, and you may end up with weird gradient shapes if the timing of gradients is not aligned.

@sravan953
Copy link
Collaborator

Thanks for the PR!

I'm in favour of option 1 because of the disadvantage with option 2 you mentioned. It seems like coming across an error about multiple gradients and then manually adding gradients is safer than passing multiple gradients and spending time debugging because the output looks different.

@FrankZijlstra FrankZijlstra force-pushed the fix_set_block branch 2 times, most recently from e10a0c7 to 8fb9dcd Compare December 11, 2023 16:08
@FrankZijlstra
Copy link
Collaborator Author

I reverted the second commit so at least the basic fix can be pushed!

@btasdelen
Copy link
Collaborator

I feel like there is no disagreement on the basic fix, so merging.

@btasdelen btasdelen merged commit dd0833d into imr-framework:dev Dec 13, 2023
4 checks passed
@FrankZijlstra FrankZijlstra deleted the fix_set_block branch January 17, 2024 14:54
@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.

3 participants