search

imr-framework / pypulseq

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

`Sequence.install` functionality #190

Closed FrankZijlstra closed 1 week ago

FrankZijlstra commented 3 months ago

This PR adds an implementation of the Sequence.install functionality as suggested in #163, with an interface that is very similar to Matlab Pulseq.

The implementation is fairly straightforward, but I wonder if the install function should throw exceptions to give more detailed errors (e.g. stdout from the system command calls).

schuenke commented 3 months ago

Didn't find the time yet, but I will hopefully review it soon

schuenke commented 1 month ago

Hey. I finally went through the code. Sorry for the loooooooooong delay @FrankZijlstra

I never used the Sequence.install() option, but hopefully be able to give it a try on Monday. Regarding the code, I think making SequenceDefinition a real Abstract Base Class would make sense. But I could do that if you want.

FrankZijlstra commented 1 month ago

Yes, using the abstract base class makes sense. But since I never used it before, you are probably able to change it quicker than me. Having can_install default to True would be nice if possible, but it wouldn't be a big deal if that method is also abstract.

schuenke commented 2 weeks ago

I tried to test the feature at our scanner, but in our institute the scanner is in a separate network so I cannot access it from my computer.

The code in general looks good to me. Did you verify that it actually works as expected @FrankZijlstra? If you confirm it, I would be willing to approve it. Or does @btasdelen wanna give it a try?

FrankZijlstra commented 2 weeks ago

Yes, I did test it (twice I believe), but only on one scanner (running Numaris X, not Numaris 4). The installation worked fine. At this point I don't remember if the caching of the scanner detection worked properly. I remember that once it kept pinging the scanner on every install call, despite being the same ipython runtime (i.e. where the cache should work). But I don't recall whether or not I fixed it afterwards... Either way that would be minor issue, that could be resolved easily later when someone is able to test it more thoroughly.

btasdelen commented 1 week ago

Our scanner is also on a separate network. The workflow we use to install is to upload the sequence to a known directory on a remote server. The scanner has access to the remote server, and we have a script to pull and install the .seq files on the scanner. Maybe I could register the remote server as a scanner? I don't know if it is a useful test, though.