ENH: Refactor actor.odf_slicer for increased performances #374
Conversation
There was a problem hiding this comment.
Thank you for this nice update @CHrlS98.
We will look into this. For backward compatibility, we might need to keep the same order of all function parameters. I wonder why the order has been changed. We do not want to break some scripts so we will need to find a way to warn the users.
Concerning the move to a new module, we plan to do it for all actors as you can see in this issue: #64.
Concerning example, there is no ODF data available somewhere. Feel free to generate it, and create a PR in https://github.com/fury-gl/fury-data. All our example datas are in this place. Then, we create a specific fetcher.
What is the size of your volume in your demo gif?
Thank again for this good work
|
Hi @skoudoro I will look into The volume shown in the screenshots is a 111 x 105 x 138 order 8 fODF SH coefficients volume of 133.0 MB. Thanks |
I agree that the sphere should be defined. So maybe we can just provide a default value instead of |
The thing is the method expects a I'll try to think of something. |
|
From look at |
|
Nice, did not know that, adding a default will be easier than I thought. |
|
Hi, |
There was a problem hiding this comment.
Hi @CHrlS98,
The code is really clean, Thank you for that. The tutorial is working well. I just have a small comment below but apart from that, I think it is ready to go.
After merging, I will tweet your gif to promote your nice work, I think it can be useful for many people and bring other contributors.
Thanks!
ps: We will fix the design of the combobox.
fury/utils.py
Outdated
| all_triangles = np.hstack( | ||
| np.c_[np.ones(len(triangles), dtype=req_dtype) * 3, | ||
| triangles.astype(req_dtype)]) | ||
| all_triangles = np.insert(triangles, 0, 3, axis=1).flatten() |
There was a problem hiding this comment.
I would recommend using req_dtype just to be sure. We had some issues in the past.
np.insert(triangles, 0, 3, axis=1).astype(req_dtype).flatten()
| def _generate_color_for_vertices(self, sf): | ||
| """ | ||
| Get array of all vertices colors. | ||
| """ | ||
| if self.global_cm: | ||
| if self.colormap is None: | ||
| raise IOError("if global_cm=True, colormap must be defined.") | ||
| else: | ||
| all_colors = create_colormap(sf.ravel(), self.colormap) * 255 | ||
| elif self.colormap is not None: | ||
| if isinstance(self.colormap, str): | ||
| # Map ODFs values [min, max] to [0, 1] for each ODF | ||
| range_sf = sf.max(axis=-1) - sf.min(axis=-1) | ||
| rescaled = sf - sf.min(axis=-1, keepdims=True) | ||
| rescaled[range_sf > 0] /= range_sf[range_sf > 0][..., None] | ||
| all_colors =\ | ||
| create_colormap(rescaled.ravel(), self.colormap) * 255 | ||
| else: | ||
| all_colors = np.tile(np.array(self.colormap).reshape(1, 3), | ||
| (sf.shape[0]*sf.shape[1], 1)) | ||
| else: | ||
| all_colors = np.tile(np.abs(self.vertices)*255, (len(sf), 1)) | ||
| return all_colors.astype(np.uint8) |
There was a problem hiding this comment.
I wonder if this block can be an independent function. I feel like I saw something similar in our codebase. I just can not remember where....
There was a problem hiding this comment.
@skoudoro How about we move it to colormap.py so it is some colormap utility function for generating a color array given some values and a colormap?
| set_polydata_triangles(polydata, all_faces) | ||
| set_polydata_vertices(polydata, all_vertices) | ||
| set_polydata_colors(polydata, all_colors) |
There was a problem hiding this comment.
there is a lot of deep copy here. I think this part can be easily optimized. There are some examples in the network modules.
For now, it is fine, but can you add a small note/TODO. It deserves its own PR
|
@skoudoro Thanks for the nice comment, On another note, I did a new GIF that I think you'll prefer. I removed the combo box and checkboxes and added transparency to the sagittal and axial slices. I also disabled the mouse capture. |
|
Ok, I tested it and it is good to go. Concerning Thank you for the updated gif. What is your Twitter handle? Concerning checkboxes, you should look at dipy_horizon. we transform the |
|
@skoudoro Alrighty thanks, See you around! |
Hi,
I did some refactoring of
actor.odf_slicerwith the objective to make it faster and less memory greedy.Basically:
forloops bynumpyarray operations;numpymethod called inutils.set_polydata_trianglefor one that completes faster;OdfSlicerActorto the new modulefury.actors.odf_slicerto make some space infury.actormodule;If there is some ODF data available somewhere, I would gladly add an example too.
Below are some screenshots showing new functionalities.
Real-time ODF volume slicing
Changing sphere resolution during execution
