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

remove total slice buffer and reuse buffer from loader #569

Conversation

frickt
Copy link
Contributor

@frickt frickt commented Sep 6, 2018

for performane reason
this commit obsoletes #548 and #558

@coveralls
Copy link

coveralls commented Sep 6, 2018

Coverage Status

Coverage decreased (-0.1%) to 62.315% when pulling 8b2e4c4 on frickt:remove_total_slices_buffer_and_reuse_from_loader into c0f2f05 on ivmartel:develop.

@ivmartel
Copy link
Owner

ivmartel commented Sep 7, 2018

Thanks for your work! My problem is that you seem to have removed the frame extra dimension, If I understand your code, you consider the first dimension to be either frame or slice. The buffer is supposed to be an array of arrays, the first dimension being the frame and then the slice. The idea is to support data as the one provided in #493.

@ivmartel ivmartel added the enhancement New feature or request label Sep 7, 2018
@ivmartel ivmartel added this to the 0.25.0 milestone Sep 7, 2018
@frickt
Copy link
Contributor Author

frickt commented Sep 7, 2018

hm... I see, didn't know that.
So should we use 3 dimensions [frameIndex][sliceIndex][buffer]?

@ivmartel
Copy link
Owner

ivmartel commented Sep 7, 2018

At the moment, there are just 2: [frameIndex][buffer]. Adding a third one could help to have an easier append slice like you do in this PR and avoid the copy.

…euse_from_loader' into remove_total_slices_buffer_and_reuse_from_loader
@frickt
Copy link
Contributor Author

frickt commented Sep 7, 2018

ok, i rewrote it to use 3d buffer.

@ivmartel
Copy link
Owner

Looking good! So how does it compare to the current version of the code? Storing the 3D data in a huge 1d array is kind of old fashion, I'm not sure there is a real benefit compared to this array of array solution.

@frickt
Copy link
Contributor Author

frickt commented Sep 10, 2018

we don't need to copy the buffer anymore, therefore gain some performance. also garbage collection is reduced. Depending on the 3d data size, the performance (for buffer handling) is improved by ((numberOfSlices - 1)/2) times.

@frickt
Copy link
Contributor Author

frickt commented Sep 26, 2018

is there somethink i need to change get this merged?

@ivmartel
Copy link
Owner

No, looking good, I was just finishing the 0.24.1patch release.

@ivmartel ivmartel modified the milestones: 0.25.0, 0.26.0 Oct 2, 2018
@ivmartel
Copy link
Owner

And now, I'm reviving the dcmbench project to evaluate your changes, I'm nearly there...

@ivmartel
Copy link
Owner

ivmartel commented Nov 5, 2018

Looking good on the default demo data of dcmbench, will integrate soon.

@ivmartel ivmartel modified the milestones: 0.26.0, 0.27.0 Apr 5, 2019
@ivmartel
Copy link
Owner

I know its an old one, but I need more time to investigate...

@ivmartel ivmartel modified the milestones: 0.27.0, Future Feb 18, 2020
@ivmartel
Copy link
Owner

Closing for now, follow #905 for new strategy.

@ivmartel ivmartel closed this Apr 21, 2021
@ivmartel ivmartel modified the milestones: Future, 0.29.0 Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants