NeuroML / pyNeuroML

A single package in Python unifying scripts and modules for reading, writing, simulating and analysing NeuroML2/LEMS models.
https://docs.neuroml.org/Userdocs/Software/pyNeuroML.html
GNU Lesser General Public License v3.0
36 stars 30 forks source link

Feature Draft/ Use PCA to visualize cell 'upwards' #379

Closed lej0hn closed 3 months ago

lej0hn commented 4 months ago
sanjayankur31 commented 4 months ago

OK for me to take a peek?

lej0hn commented 4 months ago

Yes, go ahead!

lej0hn commented 4 months ago

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

sanjayankur31 commented 4 months ago

Cool, on my list, will hopefully have a look today/tomorrow :+1:

lej0hn commented 3 months ago

I'm facing a problem with creating a new canvas in functions plot_3d_morphology and plot_3d_schematic . It seems that due to the recursion which occurs to reduce precision, the canvas created in those functions (which happens in case the canvas from plot_3d_interactive is none) , gets overwritten and can't be used

sanjayankur31 commented 3 months ago

Hrm---I think if one uses the plot_3D_morphology etc. functions directly instead of plot_intereactive_3D, we don't check for precision and reduce it etc. So we don't recurse :thinking:

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.

lej0hn commented 3 months ago

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 plot_interactive_3d?

sanjayankur31 commented 3 months ago
sanjayankur31 commented 3 months ago
lej0hn commented 3 months ago

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?

sanjayankur31 commented 3 months ago

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).

Ah, yeh, I remember that. Let me look, maybe we can implement our own orbit function based on the original implementation?

Also the XYZ shows the axis but doesn't use different colors. Any ideas?

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 VISPY_THEME dict and use them there?

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.

lej0hn commented 3 months ago

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 However, having the camera point to the y axis does make the overall turntable camera a bit more rigid when a user wants to rotate it by hand as well for some reason. If you want to check that as well

sanjayankur31 commented 3 months ago

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.

Indeed: https://vispy.org/api/vispy.visuals.xyz_axis.html#vispy.visuals.xyz_axis.XYZAxisVisual

Not sure about this. The only think I can think of is that we're using gl+, but this explicitly says gl:

https://github.com/sanjayankur31/vispy/blob/659369b37f86d44f98e413deb92df99631ef4d2a/vispy/visuals/xyz_axis.py#L27

I don't know enough to know if it's causing the issue.

So, I should drop it and change the custom way to have different colors?

Yeh, this looks simple enough, so if vispy's version doesn't do what we want, let's use our own implementation.

I'll check as well if it's possible to create our own orbit function

I did a quick search and rotating about y axis using elevation/azimuth isn't straightforward from the looks of it. Will have to do some more digging

However, having the camera point to the y axis does make the overall turntable camera a bit more rigid when a user wants to rotate it by hand as well for some reason. If you want to check that as well

Uh, hrm.

Yeh, we're sort of going against the "norm" for vispy here..

lej0hn commented 3 months ago

I think I managed to do the rotation around the y axis by using the camera's roll parameter

sanjayankur31 commented 3 months ago

Uh, so, I went ahead and grepped the code, and turns out, one can set "y is up":

https://github.com/vispy/vispy/blob/659369b37f86d44f98e413deb92df99631ef4d2a/vispy/scene/cameras/base_camera.py#L194

So, if after defining the Turntable camera, all you have to do is: cam2.up = "y", and it makes "y be up".. Sorry about that, should've grepped the code before instead of just searching the docs.

Here is the Hay et al cell (which is laid out along the Y axis originally) with just the one extra line:

https://github.com/NeuroML/pyNeuroML/assets/102575/972d29f4-d95b-4e86-8355-043635dc865b

Even orbiting "just works".

So, no need to look at orbiting and all that, and no need to make the camera face the Y axis manually---this one setting seems to handle all that.

sanjayankur31 commented 3 months ago

@lej0hn : do let me know when you want me to review this (we'll mark it as ready for review).

Also: ruff found a couple of minor issues, could you fix those too when you have second please? (see the "ruff" CI task for details)

lej0hn commented 3 months ago

Ready for review

sanjayankur31 commented 3 months ago

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:

pre-commit install

in your virtual env, and then it should run on all changed files each time you commit.

lej0hn commented 3 months ago

Looks very good.

I was testing it with another pyramidal cell here:

https://github.com/sanjayankur31/Human-L2-3-Cortical-Microcircuit/blob/feat/neuroml/NeuroML2/HL23PYR.cell.nml

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:

https://github.com/OpenSourceBrain/AllenInstituteNeuroML/blob/master/CellTypesDatabase/models/NeuroML2/Cell_329321704.cell.nml

Also not quite upright: Screencast.from.2024-06-11.16-50-16.webm

Am I doing something wrong?

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? Screenshot from 2024-06-11 19-58-30

lej0hn commented 3 months ago

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:

pre-commit install

in your virtual env, and then it should run on all changed files each time you commit.

I have already installed it

sanjayankur31 commented 3 months ago

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? Screenshot from 2024-06-11 19-58-30

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 :thinking:

Not sure how NeuroML-db have done it---I think they did it manually:

https://neuroml-db.org/gallery

sanjayankur31 commented 3 months ago

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:

pre-commit install

in your virtual env, and then it should run on all changed files each time you commit.

I have already installed it

Hrm, is it running when you commit? Check once, I thought I saw some trailing spaces that it should have removed.

lej0hn commented 3 months ago

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? Screenshot from 2024-06-11 19-58-30

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:

https://neuroml-db.org/gallery

Just by playing around with models I found that this condition works : if the z angle of rotation < 0 then do z_angle += np,pi in order to rotate it 180 degrees. There probably are cases where it doesn't apply, but maybe it satisfies the majority of them

sanjayankur31 commented 3 months ago

Just by playing around with models I found that this condition works : if the z angle of rotation < 0 then do z_angle += np,pi in order to rotate it 180 degrees. There probably are cases where it doesn't apply, but maybe it satisfies the majority of them

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.

sanjayankur31 commented 3 months ago

@lej0hn : please do let me know when you want me to take another look at this one.

github-actions[bot] commented 3 months ago

Code Coverage

Package Line Rate Health
. 53%
analysis 37%
archive 72%
channelml 55%
lems 46%
neuron 31%
neuron.analysis 0%
plot 47%
povray 0%
sbml 91%
sedml 94%
swc 8%
tellurium 0%
tune 0%
utils 62%
xppaut 0%
Summary 37% (2784 / 7612)
lej0hn commented 3 months ago

Could you take another look?

sanjayankur31 commented 3 months ago

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 :(

sanjayankur31 commented 3 months ago