Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sequence.install functionality #190

Merged
merged 4 commits into from
Nov 26, 2024

Conversation

FrankZijlstra
Copy link
Collaborator

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 Matlab Pulseq implementation for Siemens scanners is included
  • Custom scanner targets can be added using pypulseq.Sequence.install.register_scanner

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).

- The Matlab pulseq implementation for Siemens scanners is included
- Custom scanner targets can be added using `pypulseq.Sequence.install.register_scanner`
@FrankZijlstra FrankZijlstra requested a review from schuenke August 15, 2024 14:23
@schuenke
Copy link
Collaborator

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

@schuenke schuenke self-assigned this Aug 25, 2024
@schuenke
Copy link
Collaborator

schuenke commented Nov 1, 2024

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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.

@schuenke schuenke merged commit 34367e1 into imr-framework:dev Nov 26, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants