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.

Sign up for GitHub

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

Jump to bottom

fix: off-by-one error at edge of volume #22

Closed
wants to merge 4 commits into from
Closed

fix: off-by-one error at edge of volume #22

wants to merge 4 commits into from

Conversation

raj-magesh
Copy link

Closes #21.

Fixes an off-by-one error caused by conversion from MATLAB to Python indexing.

@raj-magesh
fix: off-by-one error at edge of volume
cc3621b 
Closes #21.

Fixes an off-by-one error caused by conversion from MATLAB to Python indexing.
@raj-magesh
fix: set bad voxels to NaN for complex case
99b26e8 
Not sure if this is necessary since the volumes always contain real values.
@kendrickkay
Copy link
Member

I looked over, but not 100% sure of correctness. In particular, should ">= vol.shape[0]" be "> vol.shape[0]-1", possibly?

@raj-magesh
fix: account for float coordinates
430b8cf 
Correctly handles float coordinates between `vol.shape[0] - 1` and `vol.shape[0]`
@raj-magesh
Copy link
Author

I looked over, but not 100% sure of correctness. In particular, should ">= vol.shape[0]" be "> vol.shape[0]-1", possibly?

Yup, my bad. Didn't realize that coordinates could contain floats.

@iancharest
Copy link
Collaborator

Hmm, I am not sure I follow why we set the bad coords to -1. Could we make a small test case for this such that we can compare the outputs in matlab and python?

@raj-magesh
Copy link
Author

Hmm, I am not sure I follow why we set the bad coords to -1. Could we make a small test case for this such that we can compare the outputs in matlab and python?

I just saw the 1 and assumed that any voxels set to that value were meant to be removed by the np.c_ call, but I realize that bad is included there anyway. Not sure if it's still necessary (or if it was before).

I'll write a quick test later when I get the time.

@raj-magesh raj-magesh closed this by deleting the head repository Sep 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iancharest iancharest Awaiting requested review from iancharest

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Potential bug] Interpolation wrapper appears to use 1-based indexing despite earlier correction
3 participants
@raj-magesh @kendrickkay @iancharest