search

imr-framework / pypulseq

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

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

Closed wtclarke closed 7 months ago

wtclarke commented 8 months ago

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

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

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

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.