Add line actor

[paulbrodersen]

Before submitting a pull request (PR), please read the contributing guide.

Please fill out as much of this template as you can, but if you have any problems or questions, just leave a comment and we will help out :)

Description

What is this PR

• Bug fix
• [x ] Addition of a new feature
• Other

Why is this PR needed?

3D drawing primitives are typically points, lines, and surfaces/volumes.
While there are simple ways to add points and volumes to a scene in brainrender, there seems to be no straightforward way to add a simple line to a scene. The available options seem to be to import a line via a neuron .swc morphology file, or (potentially) to import a (to me still mysterious) streamlines .json file.

What does this PR do?

This PR implements a simple Line actor class and adds it to the actors namespace.

from vedo import shapes
from brainrender.actor import Actor


class Line(Actor):
    def __init__(self, coordinates, color="black", alpha=1, linewidth=2, name=None):
        """
        Creates an actor representing a single line.

        :param coordinates: list, np.ndarray with shape (N, 3) of ap, dv, ml coordinates.
        :param color: str
        :param alpha: float
        :param linewidth: float
        :param name: str
        """

        # Create mesh and Actor
        mesh = shapes.Line(p0=coordinates, lw=linewidth, c=color, alpha=alpha)
        Actor.__init__(self, mesh, name=name, br_class="Line")

References

Could not find any related issues but also didn't peruse the tracker exhaustively.

How has this PR been tested?

This script visualizes the (pre-computed) shortest path within cortex boundaries between two cortical neurons.

import numpy as np
import vedo
vedo.settings.default_backend= 'vtk'

from brainrender import Scene
from brainrender.actors import Points, Line

scene = Scene(atlas_name="allen_mouse_25um")
cortex = scene.add_brain_region(688, alpha=0.2, color="green")

p1 = np.array([183, 202, 390]) # ABA voxel coordinates for cell #1
p2 = np.array([171, 111, 244]) # ABA voxel coordiantes for cell #2
coordinates = np.vstack([p1, p2])
coordinates *= 25 # ABA voxel resolution
cells = scene.add(Points(coordinates, name="cells", colors="steelblue", radius=100))

# add path
path = np.array([
    [183,202,390],
    [183,201,389],
    [183,200,388],
    [183,199,387],
    [183,198,386],
    [183,197,385],
    [183,196,384],
    [183,195,383],
    [183,194,382],
    [183,193,381],
    [183,192,380],
    [183,191,379],
    [183,190,378],
    [183,189,377],
    [183,188,376],
    [183,187,375],
    [183,186,374],
    [183,185,373],
    [183,184,372],
    [183,183,371],
    [183,182,370],
    [183,181,369],
    [183,180,368],
    [183,179,367],
    [183,178,366],
    [183,177,365],
    [183,176,364],
    [183,175,363],
    [183,174,362],
    [183,173,361],
    [183,172,360],
    [183,171,359],
    [183,170,358],
    [183,169,357],
    [183,168,356],
    [183,167,355],
    [183,166,354],
    [183,165,353],
    [183,164,352],
    [183,163,351],
    [183,162,350],
    [182,161,349],
    [181,160,348],
    [181,159,347],
    [180,158,346],
    [179,157,345],
    [178,156,344],
    [177,155,343],
    [176,154,342],
    [175,153,341],
    [174,152,340],
    [174,151,339],
    [173,150,338],
    [172,149,337],
    [172,148,336],
    [172,147,335],
    [171,146,334],
    [170,145,333],
    [170,144,332],
    [170,143,331],
    [169,142,330],
    [168,141,329],
    [168,140,328],
    [168,139,327],
    [168,138,326],
    [168,137,325],
    [168,136,324],
    [168,135,323],
    [168,134,322],
    [168,133,321],
    [168,132,320],
    [168,131,319],
    [168,130,318],
    [168,130,317],
    [168,129,316],
    [168,128,315],
    [168,128,314],
    [168,127,313],
    [168,126,312],
    [168,125,311],
    [168,124,310],
    [168,124,309],
    [168,123,308],
    [168,122,307],
    [168,122,306],
    [168,121,305],
    [168,120,304],
    [168,120,303],
    [168,119,302],
    [168,118,301],
    [168,118,300],
    [168,118,299],
    [168,117,298],
    [168,116,297],
    [168,116,296],
    [168,115,295],
    [168,114,294],
    [168,114,293],
    [168,113,292],
    [168,112,291],
    [168,112,290],
    [168,111,289],
    [168,111,288],
    [168,110,287],
    [168,110,286],
    [168,110,285],
    [168,109,284],
    [168,109,283],
    [168,108,282],
    [168,108,281],
    [168,108,280],
    [168,108,279],
    [168,108,278],
    [168,108,277],
    [168,107,276],
    [168,107,275],
    [168,106,274],
    [168,106,273],
    [168,106,272],
    [168,106,271],
    [168,106,270],
    [168,106,269],
    [168,106,268],
    [168,106,267],
    [168,106,266],
    [168,106,265],
    [168,106,264],
    [168,106,263],
    [168,106,262],
    [168,106,261],
    [168,106,260],
    [168,106,259],
    [168,106,258],
    [168,106,257],
    [168,106,256],
    [168,106,255],
    [168,106,254],
    [168,106,253],
    [168,106,252],
    [168,106,251],
    [168,106,250],
    [168,106,249],
    [168,107,248],
    [168,108,247],
    [169,109,246],
    [170,110,245],
    [171,111,244],
])
path *= 25 # ABA voxel resolution

# line = scene.add(Line(path))
line = scene.add(Line(path))
scene.render()

Is this a breaking change?

No.

Does this PR require an update to the documentation?

Yes, but as this is a draft PR, I haven't bothered with updating the documentation, yet.

Checklist:

• The code has been tested locally
• Tests have been added to cover all new functionality (unit & integration)
• The documentation has been updated to reflect any changes
• The code has been formatted with pre-commit

[adamltyson]

Hi @paulbrodersen, thanks for this. I've marked the PR as draft, but feel free to mark as ready for review whenever.

Please reach out if we can be of any help (you're welcome to join our Zulip chat). Some of the team are away at the moment, so we may be slower to respond than normal.

[paulbrodersen]

Hi @adamltyson, thanks for the quick feedback and the invite to Zulip.

I guess the purpose of submitting the PR in its current form was to gauge the appetite for this additional functionality, before I commit any more time understanding the testing and documentation infrastructure, and make the necessary changes there. So any feedback on that front would be welcome.

I appreciate that any additional functionality incurs maintenance costs, and that this is a particular trivial piece of code for anyone familiar with the code base. However, from the perspective of a new user -- unburdened by knowledge of any internals as I was just this morning -- it wasn't clear at all to me how to draw a line in brainrender, even after reading the entire documentation and most examples. So either the documentation needs some more love, or brainrender needs at least one more drawing primitive such as this Line class. (Or I am just blind and didn't see it, which is entirely within the realm of possibilities as well.)

[adamltyson]

So any feedback on that front would be welcome.

Sorry @paulbrodersen, I didn't say anything along those lines because I thought it was obviously a good addition!

As you say, it's a bit of an oversight to not be able to just draw a simple line. As far as I know we have lots of line-like things (neurons, cylinders, streamlines), but not actually a line!

I'm going to tag @IgorTatarnikov who has worked on the brainrender codebase more than me recently to see if he's aware of any duplication, but as far as I'm concerned this is a nice addition.

Our contributing guide is here if you haven't seen it, but we don't have any brainrender specific information yet (I've raised an issue). Ideally we would have:

• Docstrings. I can see you've added these, but I will tag @K-Meech who's working to standardise lots of BrainGlobe code in case she has any input.
• A simple test. Could be as simple as just drawing a line and checking that it ends up where it's meant to be.
• An script to go into examples. The script above is probably fine. My only suggestion would be to programmatically generate the path array, so that the script won't be so long.
• An update to the docs here. Our docs are not very comprehensive, so just adding a bullet point here should be fine. If you'd like to add anything extra to the docs though, please feel free.

Hope this helps. As I say, get in touch anytime.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.47%. Comparing base (9283ce8) to head (b365260).
Report is 15 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #334      +/-   ##
==========================================
+ Coverage   86.39%   86.47%   +0.07%     
==========================================
  Files          26       27       +1     
  Lines        1213     1220       +7     
==========================================
+ Hits         1048     1055       +7     
  Misses        165      165              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[paulbrodersen]

Summary of further changes

1. I have added a MRE using the Line actor class based on a simplified version of the example given above.
2. I have added a very simple test modeled on test/test_points.py. As the Point test, this test only tests instantiation and type.
3. I have updated the documentation with a brief indication of how to instantiate a Line actor, in line with the documentation for other actors. Related PR linked above.
4. I have not amended the docstring, and am awaiting feedback from @K-Meech.

Notes

Regarding testing, Adam's suggestion above

Could be as simple as just drawing a line and checking that it ends up where it's meant to be.

turned out to be more complicated than I thought. I couldn't identify any attributes in the underlying vedo objects that would allow me to do so, and as far as I could tell, none of the other tests tested that the data were passed through and represented correctly.

Regarding the documentation, I have kept my additions in line with the style and the level of detail of the documentation for the other actors. However, in my opinion, the whole section needs an overhaul. It completely lacks concrete examples ("Show, don't tell"), as well as any details beyond the mandatory arguments for each class. I am happy to take a first stab at fleshing out the actor documentation a bit more (in another PR) but as I am unfamiliar with many of the actor classes, this will require the commitment of at least one maintainer to proofread and to fill in the remaining gaps in the same fashion.

[adamltyson]

@paulbrodersen that all sounds reasonable to me. I will review this PR in a couple of days when I have some time to play with it properly. Thanks again!

I am happy to take a first stab at fleshing out the actor documentation a bit more (in another PR) but as I am unfamiliar with many of the actor classes, this will require the commitment of at least one maintainer to proofread and to fill in the remaining gaps in the same fashion.

Also sounds reasonable. Any contribution to the docs would be extremely valuable. Feel free to contribute whatever you can, every little helps!

[K-Meech]

Sorry for the delay in getting back to you @paulbrodersen - I put a small comment below, but the docstring looks good to me! In other parts of brainglobe we're trying to move all docstrings to numpydoc format, but as brainrender seems to universally use the :param style this fits well here.

[adamltyson]

Looks great, thanks @paulbrodersen. I've left some minor comments (sorry it's an easy PR to review so I've been very picky!), alongside @K-Meech's.

[adamltyson]

Thanks @paulbrodersen looks great! I will release a new version today.