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

Add statistics function haase claude #321

Merged
merged 222 commits into from
Jul 24, 2024

Conversation

haesleinhuepf
Copy link
Member

WIP. I'll use this to start a discussion about code with @StRigaud in a minute. General context: How can I test this?

@haesleinhuepf
Copy link
Member Author

Hey Stephane @StRigaud ,

I'm having some issues in making a detail work. The last part of the statistics function would compute standard deviation intensity and also two shape descriptors. I can't make it work, but I can narrow the problem down to the custom kernel call. Not sure what exactly goes wrong. You can reproduce my insights by comparing the temporary std-output of the C++function with the output of the python counterpart in this branch, and more precisely in this notebook.

On C++ side, after this kernel call the variable label_statistics_stack contains 0 only. But on Python side, it contains numbers. Hence, I'm suspecting something with this kernel call is wrong but I can't see the difference to the call on Python side.

image

I have to stop working on it here, because I need to work on something else before the weekend. I'm in the office next week Mon/Tue (mostly meetings though). I could spend 1-2 hours more. Maybe you see the issue and can push me in the right direction?

Thanks! And have a great weekend!

@StRigaud
Copy link
Member

I will have a look when I get 5 min :)

@StRigaud
Copy link
Member

StRigaud commented Jul 22, 2024

@haesleinhuepf So the issue was in the call of standard_deviation_per_label kernel. the parameter order to call a kernel actually matters. I miss lead you on this I think. I fix the order and now the output is good.

I also cleaned up a bit the tests. There are still some value inconsistancy making the test fail, about the minimum intensity in 3D and minimum distance to centroid value.

Idk if i should have a look, I let you update me with this but overall, we are close to something that works.

@StRigaud
Copy link
Member

I am making some little optimisation:

Instead of using the tier1::crop we can use the method copy which allow direct memory copy between GPU memory. And same a for the read and write method, can be call with a sub-region information. The only condition is that the data type remain the same, which is the case here as we are only dealing with float.

// this
tier1::crop_func(device, min_per_label, result_vector, offset, 8, 0, num_measurements, 1, 1);
// equals this
min_per_label->copy(result_vector, { num_measurements, 1, 1 }, { offset, 8, 0 }, { 0, 0, 0 });

We can actually go further and directly read the sub-region of the buffer into the CPU memory, we would skip some memory copy and operation.

// this
tier1::crop_func(device, min_per_label, result_vector, offset, 8, 0, num_measurements, 1, 1);
result_vector->read(min_intensity.data());
// equal this
min_per_label->read(min_intensity.data(), { num_measurements, 1, 1 }, { offset, 8, 0 }, { 0, 0, 0 });

But lets do this once the code is actually working correctly!

@StRigaud
Copy link
Member

It seems that the stats are correctly computed but, in 3D, the y and z values are inverted.

@StRigaud
Copy link
Member

I reverted back to classic tier1::crop for now for clarity and debuging. Let's make the thing work first before optimisation.

I have updated the tests with the correct values for 2D and 3D (relying on the python version). I believe that now we can rely on it with the objective of making the function compute correctly.

clic/src/statistics.cpp Outdated Show resolved Hide resolved
clic/src/statistics.cpp Outdated Show resolved Hide resolved
@StRigaud
Copy link
Member

With the fixed tests and the suggestion, this should work.

Missing a bit of cleaning and we are good

@haesleinhuepf
Copy link
Member Author

Hey Stephane @StRigaud ,

great, thanks for working on this! I'd love to help more, but I'm running out of time before vacation, and now my visual studio shows weird errors. I suppose, restarting it was a bad idea. Anyway, let's catch up, once I'm back from vacation in August.

big thanks again!

Cheers,
Robert

@StRigaud StRigaud merged commit 44eb68a into add-statistics-function Jul 24, 2024
1 check passed
@StRigaud StRigaud deleted the add-statistics-function-haase-claude branch August 27, 2024 14: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.

3 participants