imr-framework / pypulseq

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

pulseq 1.3.0 compatibility #36

Closed KerstinKaspar closed 3 years ago

KerstinKaspar commented 3 years ago

I am working on an application where I will probably need to read and write version 1.3 seq files (or have compatibility for both).

Are you working on the implementation of that compatibility? I am happy to contribute in any case, just wanted to check with you first.

sravan953 commented 3 years ago

Hello! Thank you for your comment. I am not actively working on version 1.3. However, it'd be great to collaborate with you and have you as a contributor to the project. If you have already started working on the code by now, please let me know.

KerstinKaspar commented 3 years ago

We will probably work on it, for now we can work around it, I will let you know!

sravan953 commented 3 years ago

Hi @KerstinHut! I completely missed out on asking you - were you enquiring about trigger support when you originally asked about Pulseq 1.3.0 compatibility? Trigger support was added to PyPulseq via PR https://github.com/imr-framework/pypulseq/pull/34 and merged in commit 27a281304f6b51c7d3350e3689221f6ca4fd3a1b. The current version of PyPulseq also supports split-gradients.

I plan to start working on incorporating Pulseq 1.3.0 changes into PyPulseq in the coming months. Please let me know if you'd like to collaborate.

KerstinKaspar commented 3 years ago

Hi @sravan953 ,

we don't need the triggers or any of the new 1.3 functionalities, actually, but rather simply needed to readboth v1.2 and v1.3 files (that also don't use any new features). We are working on pypulseq-cest, implementig CEST sequences in cooperation with the Matlab version pulseq-cest, which uses the latest 1.3 files. For compatibility of the sequence files in the simulation tools and to compare for the same outcome on the scanner, it would be preferable to produce the same files with both pulseq and pypulseq. For now, we have implemented a conversion of 1.2 seq files to a pseudo 1.3 and the other way around to allow simulations.

So it's great to hear you're working on PyPulseq 1.3.0. As for me, the next few weeks will hopefully decide whether I will get a PhD position I want and continue working in a related area. In any case I will talk to my colleagues and see if we can collaborate.

sravan953 commented 3 years ago

@KerstinHut Thanks for getting back to me! I was not aware of CEST sequences, TIL. I will begin working on transition PyPulseq to match Pulseq 1.3.x shortly; will try to get it done asap.

I'll get back to you here once I update the repo. Best of luck on your PhD decision, whichever way you decide to go! 😄

sravan953 commented 3 years ago

Hi @KerstinHut , just as an update. I will be publishing PyPulseq 1.3.1 very soon!

KerstinKaspar commented 3 years ago

That sounds great, thanks for the update!

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 @sravan953,

thanks for your work. We are checking the compatibility of pypulseq 1.3.1 with our code at the moment and found some minor bugs/errors already. What's your favorite way of handling it? Should we create a bug list or should we directly create PRs? If latter, do you prefer a single PR or individual ones for every bug/issue?

sravan953 commented 3 years ago

Oh oops, I see! I'd be glad to merge your PRs. If they are multiple but related bugs, please create a single issue and address them in a single PR. If they are unrelated, however, multiple issues/PRs would be cleaner, I suppose. In any case, I am still quite new to all of this, do you have any suggestions as to how these situations are tackled in bigger projects?

schuenke commented 3 years ago

I guess ppl often use kind of big PRs, but I also prefer to keep unrelated things separated. I still have to figure out why some things don't work as expected, but will probably create some PRs soon. As I wrote before, a lot of bugs are just minor like checking for None instead of empty string etc.

sravan953 commented 3 years ago

Alright, please open issues. We can split the tasks if there are too many.

sravan953 commented 3 years ago

Do let me know if I can close this.

KerstinKaspar commented 3 years ago

Feel free to, thanks!