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

RF: Move sigpy to an extra optional dependency #188

Merged
merged 8 commits into from
Aug 9, 2024

Conversation

wtclarke
Copy link
Contributor

@wtclarke wtclarke commented Aug 8, 2024

  • Vendor our own adiabatic pulse functions in make_adiabatic_pulse.py
  • Only enable make_sigpy_pulse.py if sigpy is installed as an extra.
  • Add test function for make_adiabatic_pulse.py
  • Add markers to disable tests on sigpy pulses if not present
  • Update the CI tests.
    Note that the Run pytest[sigpy] step in push_pr.yml is currently allowed to fail due to Incompatibility of sigpy dependency with scipy 1.14.0 and numpy 2.0 #183 .

@wtclarke
Copy link
Contributor Author

wtclarke commented Aug 8, 2024

@schuenke All good for review.

fix padding
add type hints
Copy link
Collaborator

@schuenke schuenke left a comment

Choose a reason for hiding this comment

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

I tried to refactor the code a little bit by using more descriptive variable names, introducing some variable for calculations etc...

I also realized that the padding code was not working, but I think my refactored version is a bit cleaner / easier to read anyway.

I added some additional comments. Maybe you can have a look.

pypulseq/make_adiabatic_pulse.py Show resolved Hide resolved
pypulseq/make_adiabatic_pulse.py Show resolved Hide resolved
pypulseq/make_adiabatic_pulse.py Show resolved Hide resolved
@wtclarke
Copy link
Contributor Author

wtclarke commented Aug 9, 2024

Thanks for reviewing this in depth. As a general rule, don't refactor additional code in this sort of PR as it just complicates the history. E.g. If we find a bug down the line, is it a optional variable rename or was it me removing the sigpy dep?

And remember PEP8 unto thyself, not unto others (a video well worth watching).

@schuenke
Copy link
Collaborator

schuenke commented Aug 9, 2024

Thanks for reviewing this in depth. As a general rule, don't refactor additional code in this sort of PR as it just complicates the history. E.g. If we find a bug down the line, is it a optional variable rename or was it me removing the sigpy dep?

And remember PEP8 unto thyself, not unto others (a video well worth watching).

Well, in general I agree, but the current PyPulseq code is basically not tested at all and far away from beeing stable.

Sure, I could have created another PR for the refactoring. And another one for the bug fixe(s). And maybe even a next one for the variable renaming. But in all cases our tests would have passed (although the code was not even working - see the padding code that used invalid numpy function calls) and future debugging wouldn't have been easier imo.

So for the moment, I would still refactor the code in every file that is touched by a PR anyway. To be honest, I think this will also lead to a better code review, because I can almost guarantee that nobody will ever review a PR that refactors all modules and functions. At least not in detail.

@wtclarke
Copy link
Contributor Author

wtclarke commented Aug 9, 2024

Fair points. Are you able to merge? Or is it someone else?

@schuenke schuenke merged commit fdabc22 into imr-framework:dev Aug 9, 2024
4 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.

2 participants