search

imr-framework / pypulseq

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

ValueError if triangular gradient is defined with rise_time, flat_time and amplitude #45

Closed mavel101 closed 3 years ago

mavel101 commented 3 years ago

If a triangular gradient is defined with make_trap_pulse and only amplitude, rise_time and flat_time (=0) are provided, a ValueError is raised, stating that an area has to be provided. However, the triangular gradient is fully described with rise_time and amplitude. This behaviour only occurs in the newest version, where the default value for flat_time was changed from 'None' to '0'.

sravan953 commented 3 years ago

Thank you for brining this up, had not accounted for flat_time=0 in case of triangular pulses. If the default value for flat_time is set to -1, this bug can be avoided. Can you think of any other scenarios where only this change would still fail?

mavel101 commented 3 years ago

If you set the default to -1 and the condition to "if flat_time != -1", it should be fine.

I noticed another small issue: The "set_definition" function accepts an integer as value, but in the "write_seq" an integer is only accepted as part of a list, tuple or ndarray not as a single integer. Changing this line in the write_seq

elif isinstance(values[block_counter], float):
                output_file.write(f'{values[block_counter]:0.9g} ')

to

elif isinstance(values[block_counter], (int,float)):
                output_file.write(f'{values[block_counter]:0.9g} ')

would solve this.

sravan953 commented 3 years ago

Thank you! I've made both fixes - triangular pulse and writing definitions. I am opening a separate issue (#49) for the definition bug so I can link to it a commit.

mavel101 commented 3 years ago

Perfect, thanks!

sravan953 commented 3 years ago

Please let me know if pulling from the latest dev commit fixes this issue for you.

mavel101 commented 3 years ago

Yes, both issues are fixed.