Closed paulbrodersen closed 4 months ago
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.
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.)
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:
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. Hope this helps. As I say, get in touch anytime.
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 86.47%. Comparing base (
9283ce8
) to head (b365260
). Report is 15 commits behind head on main.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Line
actor class based on a simplified version of the example given above.test/test_points.py
. As the Point
test, this test only tests instantiation and type.Line
actor, in line with the documentation for other actors. Related PR linked above. 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.
@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!
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
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 theactors
namespace.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.
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: