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

Added PNS calculations #131

Merged
merged 1 commit into from
Sep 12, 2023
Merged

Conversation

FrankZijlstra
Copy link
Collaborator

This is a direct translation of the MATLAB Pulseq calcPNS function to python, renamed to calculate_pns. This includes a partial translation of the MATLAB safe_pns_prediction repository (https://github.com/filip-szczepankiewicz/safe_pns_prediction), put into utils/safe_pns_prediction.py. To test the functionality, this adds the pnsTest sequence "example" from MATLAB Pulseq.

A few, mostly stylistic, things may need some review:

  • readasc and asc_to_hw are Siemens specific, might need to be put in a place for Siemens-specific code? (in Pulseq only readasc is separated)
  • All functions from the SAFE code are in the same file, and maintain the MATLAB comments (as far as I'm concerned it really is not meant to be more than a direct translation). And what about license terms: safe_pns_prediction is BSD-3, pypulseq is AGPL-3?
  • Location of the SAFE code (/utils) and pns_test.py (sequence examples)?

@btasdelen btasdelen self-requested a review August 28, 2023 16:27
Copy link
Collaborator

@btasdelen btasdelen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To address your questions:

  • I agree, it is better to Siemens specific functions into its own directory. If you are positive read_asc is explicitly Siemens specific, we don't have to follow Pulseq hierarchy, we can also put it in Siemens.
  • BSD is compatible with AGPL. I am not clear if it is a must, as you did modify the code significantly even though it is a translation, but we should retain the LICENSE and copyright notice for it. There are a couple of ways to do it:
  1. We can paste their license file into the file safe_pns_prediction.
  2. We can append our license with their license.
  3. We can paste the license and explain the licensing situation in the documentation.

It is your call to pick, but 1 seems the easiest.

  • I think the file positions are ok, but I would consider putting calc_pns into utils as well. Your call.

This is a direct translation of the MATLAB Pulseq calcPNS function to python, renamed to calculate_pns. This includes a partial translation of the MATLAB safe_pns_prediction repository (https://github.com/filip-szczepankiewicz/safe_pns_prediction), put into utils/safe_pns_prediction.py. To test the functionality, this adds the pnsTest sequence "example" from MATLAB Pulseq.
@FrankZijlstra
Copy link
Collaborator Author

I moved the Siemens functions into utils/siemens and added the BSD license into safe_pns_prediction.
I did not move calc_pns to remain consistent with MATLAB pulseq (similar as for ext_test_report, read_seq, etc.)

I updated the previous commit to keep the pull-request to a single commit.

@sravan953 sravan953 self-requested a review September 12, 2023 14:57
@sravan953 sravan953 added the enhancement New feature or request label Sep 12, 2023
@btasdelen btasdelen merged commit c251d77 into imr-framework:dev Sep 12, 2023
@FrankZijlstra FrankZijlstra deleted the calc_pns branch January 8, 2024 13:56
@schuenke schuenke mentioned this pull request Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants