make_trapezoid with just flat_area fails

[gabuzi]

Describe the bug Calling pypulseq.make_trapezoid with only flat_area given fails. Specifying just the area argument works fine.

To Reproduce

import pypulseq as pp
pp.make_trapezoid('x', flat_area=1)
...
File "...lib/python3.10/site-packages/pypulseq/make_trapezoid.py", line 162, in make_trapezoid
  np.ceil(np.sqrt(np.abs(area) / max_slew) / system.grad_raster_time)
TypeError: bad operand type for abs(): 'NoneType'

Expected behavior Should return a trapezoid gradient event with the requested flat_area.

Desktop (please complete the following information):

• OS: macOS
• OS Version: 13.6.2
  • pypulseq version: 1.4.0

Looking at make_trapezoid.py, there are checks like if area == 0: raise ValueError ("Must supply area or duration."), which appears out of date as the default value for area is None in 1.4.0.

So it looks like just supplying flat_area was never intended to work, but the error is not caught anymore. But rather than catching the absence of area as an error, I propose to allow just specifying flat_area and return a trapz gradient with default slew rates having the intended flat_area.

[gabuzi]

There's more: If area, rise_time & fall_time (but not flat_time) are given, rise and fall times are ignored. This seems to be due to the the else branch of the if area == 0 check mentioned in the issue above ignoring user-provided rise & fall times and recomputing them.

[btasdelen]

Hi @gabuzi, thanks for reporting this. It is definitely an unintended behavior.

Are you suggesting just supplying flat_area should give the shortest flat area gradient, just like only providing the area does? Do you have any use cases for asking the shortest flat area gradient?

I think if it is not something that most developer expect, it might not be best to silently give something without throwing a warning.

[gabuzi]

Hi!

Thanks for following up!

To be quite honest, I stumbled over it by mistakenly using flat_area instead of just area, so I don't actually have a real use case for that anymore. Giving it more thought, it's probably rather unusual to request a trapz gradient with a specific flat_area, but not caring about the flat duration. So I guess it's probably not a huge loss if that wouldn't be supported. The same probably applies to the issue in the second comment about giving specific rise and fall times but not the flat time.

I totally agree, optional parameters should never be silently ignored, but rather result in an error if they won't be honored.

[wtclarke]

I think I've made some positive changes to handling arguments in this function in PR #146 , but it would be great for the devs to have a look at the proposed tests to confirm I've interpreted the use cases correctly.

Also see #145

[btasdelen]

@wtclarke I'm on it. I checked most of it and it looks reasonable. I will check the rest and merge it.

@gabuzi I realized that issue you mentioned is already solved in #128. You can pull the changes to get the fix. Further fixes will be applied when #146 gets merged.

[gabuzi]

@btasdelen Thanks a lot and sorry for the dupe!