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

BF: make_trapezoid with amplitude + duration as inputs. #146

Merged
merged 3 commits into from
Nov 14, 2023

Conversation

wtclarke
Copy link
Contributor

@wtclarke wtclarke commented Nov 1, 2023

This should fix the issue described in #145. It also adds partial tests for the make_trapezoid function. Finally it removes a unused inport that was throwing warnings with tests in the pypulseq/SAR/SAR_calc.py module.

@wtclarke
Copy link
Contributor Author

wtclarke commented Nov 1, 2023

Before this is merged, it would be great to understand exactly what the calling cases should be so that we can write some proper tests for them.

@btasdelen
Copy link
Collaborator

@wtclarke I checked the PR, and the use cases you documented covers pretty much all the "useful" and well-defined cases I can think of.

    The user must supply as a minimum one of the following sets:
    - area
    - amplitude and duration
    - flat_time and flat_area
    - flat_time and amplitude
    - flat_time, area and rise_time

The complexity of input parameter sets bothers me, though. It is error-prone and not really Pythonic. I know it is something inherited from the Matlab Pulseq, so I don't know how to solve it without breaking the familiarity.

Are you considering adding test cases, or should I go ahead and merge?

@wtclarke
Copy link
Contributor Author

If you are happy with the use cases then go ahead and merge. These are all already tested for in test_make_trapezoid.py. There's still lots that should be added to that test set, but this is a start.

@btasdelen btasdelen merged commit 799b3eb into imr-framework:dev Nov 14, 2023
0 of 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.

None yet

2 participants