search

imr-framework / pypulseq

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

float compatibility problems #37

Closed KerstinKaspar closed 3 years ago

KerstinKaspar commented 3 years ago

For our sequence design of long sequences, we're experiencing problems with the np.float16 declaration in the compress_shape function (line 32 of compress_shape.py).

Could you change it to np.float32 or np.float_? That resolves the issue.

sravan953 commented 3 years ago

Thank you for bringing this up @KerstinHut. Could you please share code to reproduce this error?

KerstinKaspar commented 3 years ago

Sorry, I was a little lazy to produce a small example.

The following code produces the float16.seq file I attached, where shape 2 is cut 12 samples short (at the very end of the file). When I change the np.float16 to np.float32 in the compressed shape function, the output is the float32.seq file, which is correct. (I have to pass them as .txt)

float16.txt float32.txt

import math
from pypulseq.Sequence.sequence import Sequence
from pypulseq.make_gauss_pulse import make_gauss_pulse
from pypulseq.opts import Opts

seq = Sequence()

b1 = 2.22
t = 50e-3

seq_filename = 'float16.seq'

# scanner limits
sys = Opts(max_grad=40, grad_unit='mT/m', max_slew=130, slew_unit='T/m/s', rf_ringdown_time=30e-6, rf_dead_time=100e-6,
           rf_raster_time=1e-6)
gamma = sys.gamma * 1e-6

rf_sat, _, _ = make_gauss_pulse(flip_angle=b1 * gamma * 2 * math.pi * t, duration=t, system=sys, time_bw_product=3)

seq.add_block(rf_sat)

seq.write(seq_filename)
schuenke commented 3 years ago

Hey @sravan953, I'm a colleague of @KerstinHut and just wanted to give a short explanation why we need such long rf pulses: we wanna use (py)pulseq for magnetization prepared sequences like Chemical Exchange Saturation Transfer (CEST) or Spinlock/T1rho. Especially in CEST rf pulse durations of up to several seconds are pretty common.

schuenke commented 3 years ago

Hey, I just wanted to check back on this Issue. We are using pypulseq for several projects at the moment and changing the float type would allow us to use it directly as a submodule in our repos. Would be great!

mzaiss commented 3 years ago

This would be also a very important bug fix for us!

sravan953 commented 3 years ago

Hello! Sorry for the delay, this went under my radar. I have just pushed a commit to the dev branch. To install from dev branch: pip install git+https://github.com/imr-framework/pypulseq.git@dev

sravan953 commented 3 years ago

Hello @KerstinHut , please let me know if things worked out alright?

KerstinKaspar commented 3 years ago

Hello @sravan953 , I am sorry I haven't been in touch, I have been busy on another project and forgot to try. The long pulses seem to work now, thank you! I also noticed there is an EXT column in our files now? If I saw correctly, these are also just zeros? We have implemented something similar and can use yours as well, now. Is there anything else we should know about the current dev branch? Thank you for your help!

sravan953 commented 3 years ago

No problem, just wanted to check in and see if everything was alright. Yes the EXT column is only on the dev branch for now, for external triggering support (see https://github.com/imr-framework/pypulseq/pull/34).

Do let me know if this issue can be closed.

KerstinKaspar commented 3 years ago

Great, thank you!

schuenke commented 3 years ago

Hey @sravan953, we would like to upload our package pypulseq-cest (that requires the pypulseq dev version with the float fix) to PyPi. However, PyPi doesn't allow direct dependencies so that we cannot install pypulseq from the dev branch. (To make it clear: pip install git+https://github.com/imr-framework/pypulseq.git@dev works, but setting it as dependency in the setup.py of our package does not!) As we would like to avoid including a copy of pypulseq in our package, I wanted to ask if you could a) push this hot fix to the main branch and release it (e.g. as new maintenance version 1.2.1) so that it we can just set pypulseq as dependency for our package b) upload the dev-branch (e.g. as pypulseq-dev) to PyPi as well, so that we can use this as dependency. I could do this on my own, but wanted to ask you first and ofc I would also prefer option a) over b)

Best Patrick

sravan953 commented 3 years ago

Hi @schuenke thanks for bringing this to my notice! :) I will figure out the best way to do this and get back to you!

sravan953 commented 3 years ago

Hello @schuenke! Hope you are all doing well. I pushed the changes you requested to the main branch and also as a PyPI release. I didn't find time to do any testing, hopefully nothing breaks 😬! Please take a look - https://github.com/imr-framework/pypulseq/releases/tag/1.2.0post4 Let me know if it works :)