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

Fix violation check rule #177

Merged
merged 2 commits into from
Aug 9, 2024
Merged

Fix violation check rule #177

merged 2 commits into from
Aug 9, 2024

Conversation

chaithyagr
Copy link
Member

No description provided.

@chaithyagr chaithyagr requested a review from Daval-G August 6, 2024 14:10
@paquiteau
Copy link
Member

Why don't you just negate the array in your code ? :D

@chaithyagr
Copy link
Member Author

Ahhh you want to have bugs to fix bugs, nice xD
Shall i change this line

if violation:
warnings.warn(
"Hard constraints violated! "
f"Maximum gradient amplitude: {maxG:.3f} > {gmax:.3f}"
f"Maximum slew rate: {maxS:.3f} > {smax:.3f}"
)

to if not violation:, then warn? :P

@paquiteau
Copy link
Member

or s/violation/valid/ and then if not all(valid): ?

@chaithyagr
Copy link
Member Author

Just approve and merge bro xD

@chaithyagr
Copy link
Member Author

Lol I didn't think this PR will have su much discussion xD I thought it would be, oops missed that

@Daval-G Daval-G added the question Further information is requested label Aug 8, 2024
@Daval-G
Copy link
Collaborator

Daval-G commented Aug 8, 2024

I tend to agree with @paquiteau, why change this function when it is just misused ? We can surely discuss whether those functions should check validity or violation, but for now the only bug is in mri-nufft/src/mrinufft/io/nsp.py

@chaithyagr
Copy link
Member Author

I give up xD

@chaithyagr chaithyagr added answer and removed question Further information is requested labels Aug 8, 2024
@paquiteau paquiteau merged commit aad8c0e into master Aug 9, 2024
11 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