imr-framework / pypulseq

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

Return values of make_xxx_pulse functions #47

Closed schuenke closed 3 years ago

schuenke commented 3 years ago

Since https://github.com/imr-framework/pypulseq/commit/7542b39630c1c4cefc6eb5ded3ddb290a54e487c the make-pulse-functions like _make_sincpulse.py and _make_gausspulse.py return a different number of variables as before when using return_gz = False.

For compatibility with previous versions I suggest to return Nones for gz and gzr as it was the case before the release of v1.3.1. However, I think using the _return_gz' flag instead of the try block is a good idea and I suggest to change the return lines only. For example the _make_sincpulse.py could be modified to:

if return_gz:
    return rf, gz, gzr
else:
    return rf, None, None

This would also solve issue https://github.com/imr-framework/pypulseq/issues/46

Let me know if I should include this in the PRs for issue https://github.com/imr-framework/pypulseq/issues/46 and issue https://github.com/imr-framework/pypulseq/issues/48.

sravan953 commented 3 years ago

I guess the return function executes irrespective of the return_gz value, missed this! I will work on this fix, and incorporate your suggested changes, consequently fixing #46.

sravan953 commented 3 years ago

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

sravan953 commented 3 years ago

Thanks @schuenke for bearing with me. The latest commits must have fixed this for good!

schuenke commented 3 years ago

Issue is fixed now. Thanks @sravan953 !

schuenke commented 3 years ago

Hey @sravan953, will you merge this fix and the others from the dev branch into master and create a new release soon?

sravan953 commented 3 years ago

Hi, yes! I commit to publishing a new release by Friday (04/30). Thanks for bearing with me!

schuenke commented 3 years ago

Will this be 1.3.1post1 than? We need to submit the proofs of our paper by tomorrow and I would lile to mention the release.

sravan953 commented 3 years ago

Yes 1.3.1post1

sravan953 commented 3 years ago

@schuenke Hi, published!

schuenke commented 3 years ago

Thanks @sravan953 . Will test it tomorrow 👍

schuenke commented 3 years ago

Seems to work fine. Thanks.