Feature Draft/ Use PCA to visualize cell 'upwards' #379
Conversation
lej0hn
commented
Jun 3, 2024
lej0hn
commented
- Finds the first PCA
- Finds rotations needed to bring it along z axis
- Rotates the cell using same angles
- Translates cell to origin
- Visualizes axes and PCA axis (before and after rotation)
|
OK for me to take a peek? |
|
Yes, go ahead! |
|
The code is in different functions right now and I added the extra parameters just to be able to temporarily visualize the pca axis as well |
|
Cool, on my list, will hopefully have a look today/tomorrow 👍 |
There was a problem hiding this comment.
Looking very good! I think we're almost there. I've dropped a few minor comments.
I think the camera isn't still pointing to origin, and we're still slightly off---see the rotation bit:
Screencast.from.2024-06-04.12-25-47.webm
I'm not sure why this is given that youre axes are shown about the origin. Maybe the bounding box calculation is off by a bit, so the camera is slightly askew too?
| @@ -471,6 +486,50 @@ def plot_interactive_3D( | |||
|
|
|||
| else: | |||
| cell = list(pop_id_vs_cell.values())[0] | |||
| #Get all segments' distal points | |||
| segment_points = [] | |||
There was a problem hiding this comment.
I wonder if we can move this functionality into the plot_3D_cell_morphology function, and add an argument there to activate this transformation (instead of passing x_angle and y_angle pass: pcaify=True (with a better argument name, of course :))? I wonder because this is limited to the case where we're plotting a single cell, so it sort of fits better there. One should also be able to use this when using plot_3D_cell_morphology directly?
It'll also mean that the rotation + translation only happens once. At the moment, in the hay model, for example, it's happening multiple times---each time the mesh precision is dropped.
There was a problem hiding this comment.
Yes this is the goal, this just allowed for visualization of the pca axes for debugging. However, besides not being able to visualize the pca axes after this change, I think there is another issue with the camera, as you noted above. So, the bounding box that's used to center the camera is taken in plot _interactive_3d , before the cell is translated to origin or rotated, so the bounding box and therefore the camera, is centered on the original cell's position. Since in this case we're translating the cell to the origin, maybe along with pcaify=True , we could set an extra argument in create_new_vispy_canvas that sets the view_center of the camera at the origin as well?
There was a problem hiding this comment.
Yeh. So, if there's only one cell, we don't need to call create_new_vispy_canvas in plot_3D_interactive at all. I can't remember why I did it there. It doesn't seem necessary. We could conditionalise it there so that it's only called when there's more than one cell to be plotted, otherwise we let plot_3D_cell_morphology or plot_3D_schematic do it when it finds that current_canvas and current_view are None.
I just remembered that we also have the plot_3D_schematic function too, where it'll also be good to apply the PCA method so that the schematic is also upright. So perhaps we need to split out the PCA bits into a new method that takes a cell as argument and returns the "upright" cell? This could be called by both plot_3D_cell_morphology and plot_3D_schematic before they create the canvas and the meshes etc?
Yes, we could add another optional argument to create_new_vispy_canvas called view_center, which if provided will be used. If it isn't, we'll calculate the centre from view_min and view_max?
How does that sound?
There was a problem hiding this comment.
Yeah sound great. On it
| @@ -769,6 +832,9 @@ def plot_3D_cell_morphology( | |||
| except Exception: | |||
| axon_segs = [] | |||
|
|
|||
| cell = rotate_cell(cell, x_angle, y_angle, 0, 'yxz', False, False) | |||
| cell = translate_cell_to_coords(cell, False, [0,0,0]) | |||
There was a problem hiding this comment.
I assume the order of operations is important---would it be different if we:
- first translated the cell to origin
- did the pca
- rotated the cell about the soma (origin)?
There was a problem hiding this comment.
- first translated the cell to origin
- did the pca
These two steps can be done in any order ,I believe, without any change because the PCA first translates the points so that their center is at the origin and then applies other computations.
But after that, the best way is in fact to translate and then rotate, now that I think of it, because the rotations and the angles are calculated based on the PCA axis, which starts at the origin. So to apply them succesfully to the cell, it should be close to the origin in order to have similar results.
|
I'm facing a problem with creating a new canvas in functions |
|
Hrm---I think if one uses the I'll try to take a look at your PR today, otherwise we can discuss it at the meeting tomorrow and see what to do. |
|
I didn't check for using them directly, so you are probably right at that. Perhaps it is a bit of a hustle, however, for the user, to call these functions directly instead of handling it through |
|
|
|
I am facing an issue with the rotation we discussed around the y axis. There was no problem getting the camera to look at the y axis and make the cell align to it, but I can not find a way to make the camera orbit around the y axis. The orbit function has only 2 arguments, elevation and azimuth which rotate it around the x and z axis respectively ( https://vispy.org/api/vispy.scene.cameras.turntable.html#vispy.scene.cameras.turntable.TurntableCamera.orbit). Also the XYZ shows the axis but doesn't use different colors. Any ideas? |
Ah, yeh, I remember that. Let me look, maybe we can implement our own orbit function based on the original implementation?
The XYZ from vispy, or our custom code? In the custom code, we set the colour to the "fg" for all lines, but we should be able to specify a colour per line there. You could define three colours for the XYZ axes for the light and dark themes in the See: https://vispy.org/api/vispy.scene.visuals.html#vispy.scene.visuals.Line for usage PS: could you merge development into your branch again? I added coverage and made some linter fixes to make ruff happy. |
|
I used XYZ from vispy and made it work, but the colors are all the same. The default is supposed to be different for each axis, and even when I specified colours it didn't work. So, I should drop it and change the custom way to have different colors? I'll check as well if it's possible to create our own orbit function |
|
Ready for review |
There was a problem hiding this comment.
Looks very good.
I was testing it with another pyramidal cell here:
It's translated correctly, but the rotation doesn't look right. Do you want to try to see why? It's not quite "upright", but should be given the morphology? 🤔
Screencast.from.2024-06-11.16-45-08.webm
Here's another example from the Allen cell types database:
Also not quite upright:
Screencast.from.2024-06-11.16-50-16.webm
Am I doing something wrong?
|
|
||
| if isPCA: | ||
| cell = PCA_transformation(cell) | ||
| current_canvas = current_view = None |
There was a problem hiding this comment.
is this meant to be in the conditional? otherwise the canvas and view are always being cleared?
| """ | ||
| if title == "": | ||
| title = f"3D schematic of segment groups from {cell.id}" | ||
|
|
||
| if isPCA: | ||
| cell = PCA_transformation(cell) |
There was a problem hiding this comment.
do we need to clear the canvas here, and set the camera to origin etc?
| @@ -745,6 +845,8 @@ def plot_3D_cell_morphology( | |||
| is used) | |||
|
|
|||
| :type highlight_spec: dict | |||
| :param isPCA: bool that if True, transforms single Cell according to its PCA to make it look 'upwards' | |||
| :type isPCA: bool | |||
There was a problem hiding this comment.
probably worth adding the axes_pos argument here too, so that users can activate it when viewing single cells too (same for schematic figure)?
There was a problem hiding this comment.
axes_pos argument doesn't exist in plot_interactive_3d either, should I add in every one of those?
There was a problem hiding this comment.
Ah, yeh. We can think about whether we want to also expose this in the pynml-plotmorph command line tool later.
|
One more thing---do check if the pre-commit hook is running for you. It should remove empty lines and trailing spaces etc. Try running: in your virtual env, and then it should run on all changed files each time you commit. |
No, you were right, there was a missing '-' sign in the angles. But that reminds me, in the case of the first cell for example, do we wish to see it like this, or opposite? |
I have already installed it |
This is the correct way---that long straight line is an artificial axon that the modellers have added there. The apical dendrites here are correctly going up. I don't know how to generalise this mathematically though. We can't say "soma is always down", and we can't necessarily say "long dendrites always go up" because that may not be the case for all cells 🤔 Not sure how NeuroML-db have done it---I think they did it manually: |
Hrm, is it running when you commit? Check once, I thought I saw some trailing spaces that it should have removed. |
Just by playing around with models I found that this condition works : |
Cool, let's go with that for now. Please do document it in the docstring I guess, just so users are aware that this is how we do it. |
|
@lej0hn : please do let me know when you want me to take another look at this one. |
…gy and plot_3d_schematic functions
|
|
Could you take another look? |
|
Sure, will try it today, but we've got a divisional meeting coming up so it may have to be tomorrow during our catch up :( |
