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

Assert that make_trapezoid() slew rate is smaller than max_slew #214

Merged
merged 3 commits into from
Dec 11, 2024

Conversation

h3lg3
Copy link
Contributor

@h3lg3 h3lg3 commented Dec 4, 2024

Tries to address #213

make_trapezoid() checks at the end if system.max_slew is exceeded. Similar to what is done with system.max_grad.

@h3lg3 h3lg3 changed the title * fix: exceed of max slew rate for gradient event if make_trapezoid()… Assert that make_trapezoid() slew rate is smaller than max_slew Dec 4, 2024
@FrankZijlstra
Copy link
Collaborator

Yes, I agree that this should be checked. In addition, the same check should be done for the ramp down, i.e. using fall_time instead of rise_time.

And is it possible to create a gradient with a rise_time of 0.0 (and consequently, an amplitude of 0.0)? If so, the assertion needs to check that to avoid a division by 0.

@schuenke How do we deal with the ruff error in the pre-commit here? Should @h3lg3 run it locally to fix the formatting, or just change the formatting as suggested in the error log (https://github.com/imr-framework/pypulseq/actions/runs/12162263429/job/34029382533?pr=214)?

@schuenke
Copy link
Collaborator

schuenke commented Dec 7, 2024

@schuenke How do we deal with the ruff error in the pre-commit here? Should @h3lg3 run it locally to fix the formatting, or just change the formatting as suggested in the error log (https://github.com/imr-framework/pypulseq/actions/runs/12162263429/job/34029382533?pr=214)?

Ideally @sravan953 will activate pre-commit.ci for PyPulseq. If this is done, it would be fixed automatically. I asked him at GitHub and Slack, but didn't get a response yet. So for the moment it would be either @h3lg3 fixing it or one of us. Will probably take me 30 secs, so fine for me.

@schuenke
Copy link
Collaborator

schuenke commented Dec 9, 2024

pre-commit.ci autofix

@schuenke
Copy link
Collaborator

schuenke commented Dec 9, 2024

Okay, @sravan953 activated pre-commit ci and it seems to work fine.

@FrankZijlstra FrankZijlstra merged commit b4a59ce into imr-framework:dev Dec 11, 2024
6 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