-
Notifications
You must be signed in to change notification settings - Fork 64
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
Conversation
@schuenke All good for review. |
fix padding add type hints
There was a problem hiding this 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.
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. |
Fair points. Are you able to merge? Or is it someone else? |
make_adiabatic_pulse.py
make_sigpy_pulse.py
if sigpy is installed as an extra.make_adiabatic_pulse.py
Note that the
Run pytest[sigpy]
step inpush_pr.yml
is currently allowed to fail due to Incompatibility of sigpy dependency with scipy 1.14.0 and numpy 2.0 #183 .