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

A major bug in NSP read trajectory implementation #85

Merged
merged 4 commits into from
Jul 1, 2024

Conversation

chaithyagr
Copy link
Member

This bug has wide implications. Bascially when OSF>1 (which is usually the case), for the first 0, 1, ... OSF points for each shot, we end up with bad k-space trajectory location which is wrong!

@chaithyagr chaithyagr requested a review from paquiteau March 6, 2024 13:39
@chaithyagr
Copy link
Member Author

Noting here that the issue mostly affects center-out trajectories adversly

@chaithyagr chaithyagr changed the title A major bug in SPARKLING read trajectory implementation A major bug in NSP read trajectory implementation Mar 6, 2024
@paquiteau
Copy link
Member

good catch ! you might want to lint your code however ;)

@chaithyagr
Copy link
Member Author

I just thought i did xD didnt commit damn it.

@chaithyagr
Copy link
Member Author

We might want to test this use case, which is very common. Possibly add a test which reads an interpolated trajectory and compare if the interpolation matches to what we expect

@chaithyagr chaithyagr closed this Apr 10, 2024
@chaithyagr chaithyagr deleted the fix_nsp branch April 10, 2024 08:28
@chaithyagr
Copy link
Member Author

This is bizzare, I thought this was merged! @paquiteau any idea how we missed this, can I merge it?

@chaithyagr chaithyagr restored the fix_nsp branch July 1, 2024 09:00
@chaithyagr chaithyagr reopened this Jul 1, 2024
@chaithyagr chaithyagr merged commit d2e9e53 into mind-inria:master Jul 1, 2024
8 checks passed
@chaithyagr chaithyagr deleted the fix_nsp branch July 1, 2024 10:58
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