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

BUG: Need to fill buffer for R and Python interfaces. #1699

Merged
merged 1 commit into from
Mar 18, 2024
Merged

Conversation

ntustison
Copy link
Member

@stnava
Copy link
Member

stnava commented Mar 18, 2024

damn - that's a classic one and hard to "see" ...

@stnava
Copy link
Member

stnava commented Mar 18, 2024

guess it also raises the question of where else this may occur .....

@stnava stnava closed this Mar 18, 2024
@stnava stnava reopened this Mar 18, 2024
@cookpa
Copy link
Member

cookpa commented Mar 18, 2024

guess it also raises the question of where else this may occur .....

There's a few other instances of Allocate() not followed by FillBuffer, looks like mostly in ImageMath. There's also some code that calls AllocImage, which has functions with and without FillBuffer.

Is there an explanation of how this works? It seems the uninitialized data is all internal to the C++ code, why does it not cause non-deterministic behavior when run directly?

@stnava
Copy link
Member

stnava commented Mar 18, 2024

my understanding ( from itk discussions ) is the C++ standard does not define expectations for values within uninitialized memory; such values are indeterminate, and accessing them leads to undefined behavior. ... not sure if itk Allocate is supposed to behave differently though

@gdevenyi
Copy link
Contributor

To fix this common issue itk has been transitioning to a single call for both

InsightSoftwareConsortium/ITK#4494

@cookpa
Copy link
Member

cookpa commented Mar 18, 2024

Thanks @gdevenyi - so it looks like we could replace Allocate() with Allocate(true) or, as of that PR, AllocateInitialized() and solve this problem consistently.

@stnava
Copy link
Member

stnava commented Mar 18, 2024

yes - worthwhile. would also need to check all the R c++ and python c++ ( not much in the latter , I think ) .... another issue is I have not updated pybind11 in some time .... not sure if this would be related or not.

@ntustison
Copy link
Member Author

I'll go ahead and merge this and a future pull can address the more general presence of this issue.

@ntustison ntustison merged commit 0ec3fa5 into master Mar 18, 2024
2 checks passed
@ntustison ntustison deleted the KKFillBuffer branch March 18, 2024 14:08
@ntustison
Copy link
Member Author

Unless somebody else wants to do it, I can put it on my to-do list to go through and check that the image buffer is initialized.

@cookpa
Copy link
Member

cookpa commented Mar 18, 2024

Thanks Nick, much appreciated. I'll make some issues so we can keep track of this

@ntustison
Copy link
Member Author

Thanks @cookpa . But just to be clear, it might take a couple days to a week.

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.

4 participants