search

imr-framework / pypulseq

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

Missmatch between compress_shape function in Python and MATLAB #42

Closed schuenke closed 3 years ago

schuenke commented 3 years ago

In 2268bf670e68c59404c1c322f2ce12b0e00c7439 you changed the quantization factor in _compressshape.py from 1e-7 (as in MATLAB) to 1e-8. This leads to deviations between seq-files created in MATLAB and python.

For our pulseq-cest project it is important that the seq-files created with MATLAB and python match as good as possible. Therefore, we would prefer to change the factor back to 1e-7.

Is/was there a specific reason why you decided to deviate from the MATLAB implementation in this case and is there any change that you change it back to the old value?

Unfortunately this issue is pretty urgent for us, because we need to create a release for a publication.

Thanks again for your time and help!

Best, Patrick

sravan953 commented 3 years ago

Hi Patrick! Thank you for bringing this to my notice. I am currently working on PyPulseq 1.3.1 to match Pulseq 1.3.1. I understand your concern and urgency. If you have tested with 1e-8 and it works as expected for you, please make a PR to the dev branch. I will merge changes asap and get back to you.

sravan953 commented 3 years ago

I've merged the PR into the dev branch. You must be able to install the latest commit from the dev branch as follows: pip install git+https://github.com/imr-framework/pypulseq.git@dev.

Let me know if it works! :)

schuenke commented 3 years ago

Thanks for the fast response and fast merge. The compressed shapes in the seq-files created in python match the matlab version now! Great!

Is there any chance that this minor change makes it into a release 1.2.0post5 etc in the very near future?

I would love to list pypulseq (including the change) as an install requirement in the setup.py of our python package. Although listing git branches as requirement does work now (!), it is still kind of a work around and of course requires the user to have Git installed. To keep our project as simple and user friendly as possible, it would be amazing if we could avoid this Git requirement.

Thanks again!

sravan953 commented 3 years ago

I am on track to releasing PyPulseq 1.3.1 very soon. Hopefully in the next 7 days or sooner!

schuenke commented 3 years ago

Sounds good! Just to be sure: will you definetely include this fix in the next release? Would be good to know if we can mention it in the publication.

sravan953 commented 3 years ago

Yes 100%!

sravan953 commented 3 years ago

Hi! I've just released the latest version. Please take a look and let me know if I can close this issue. :)

schuenke commented 3 years ago

Hey! The shape compression works perfect with the latest version. You can close the issue. However, we found some other minor bugs/errors as I wrote in https://github.com/imr-framework/pypulseq/issues/36. Let me know how you prefer me to report/contribute.