Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Brainstem #128

Merged
merged 13 commits into from
May 27, 2021
Merged

Brainstem #128

merged 13 commits into from
May 27, 2021

Conversation

s-fong
Copy link
Contributor

@s-fong s-fong commented Mar 28, 2021

No description provided.

Copy link
Member

@rchristie rchristie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The terms file is missing (please also rename - see comment) and a syntax error meaning I can't run it to check. Please check it is working for you.
I realise you're moving on so some of these task may not need to be done by you (may be done later), but it would be good to get it running properly and in the codebase.

src/scaffoldmaker/scaffolds.py Outdated Show resolved Hide resolved
src/scaffoldmaker/meshtypes/meshtype_3d_brainstem.py Outdated Show resolved Hide resolved
src/scaffoldmaker/meshtypes/meshtype_3d_brainstem.py Outdated Show resolved Hide resolved
src/scaffoldmaker/meshtypes/meshtype_3d_brainstem.py Outdated Show resolved Hide resolved
src/scaffoldmaker/meshtypes/meshtype_3d_brainstem.py Outdated Show resolved Hide resolved
Comment on lines 228 to 236
eIndexPM = {}
xiPM = {}
pointMarkers = {}
eIndexPM['caudal-dorsal'] = int(elemPerLayer/2)
eIndexPM['midRostCaud-dorsal'] = int(elemPerLayer / 2) + (elemPerLayer * int((elementsCountAlong/2)-1))
eIndexPM['rostral-dorsal'] = (elemPerLayer*(elementsCountAlong-1)) + int(elemPerLayer/2)
eIndexPM['caudal-ventral'] = int(elemPerLayer/2) - (elementsCountAcrossMinor-1)
eIndexPM['midRostCaud-ventral'] = eIndexPM['midRostCaud-dorsal'] - (elementsCountAcrossMinor-1)
eIndexPM['rostral-ventral'] = int((elemPerLayer*(elementsCountAlong-1)) + (int(elemPerLayer/2) - elementsCountAcrossMinor + 1))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're now adding the marker points as annotation groups and putting the points in not just the marker group, but the brainstem group and their own annotation group.
We will add the point annotations with the name and term Id "None" (a string with the word None in it) and request the terms from SciCrunch using the google sheet the mappers maintain.
All the names need to be qualified with "brainstem" and point/junction so they are clear what anatomy they are for in SciCrunch. Since I can't run it yet, I can't comment any further on the locations. Avoid abbreviations like midRostCaud.

@s-fong
Copy link
Contributor Author

s-fong commented May 12, 2021

I have pushed these changes. The mesh file I have added includes updated markers for where cranial nerves 3-12 emerge from the brainstem, which was not considered at the time of this original pull request.

@s-fong
Copy link
Contributor Author

s-fong commented May 12, 2021

Sorry, I have pushed the wrong version up. Please don't review this one yet, it won't work!

@s-fong
Copy link
Contributor Author

s-fong commented May 12, 2021

This should work for you now. However, cranial nerve emergent markers are not included as they are computed in "brainstem_coordinates" which are not defined on this mesh.

Copy link
Member

@rchristie rchristie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some feedback. We need to have a discussion/handover.
I'd like to get some idea of the sizes and shapes in different species.
Is the midbrain normally longer than that?
Can we make annotation groups for the caudal end where it attaches to the spinal cord, and the cranial end?

src/scaffoldmaker/meshtypes/meshtype_3d_brainstem.py Outdated Show resolved Hide resolved
src/scaffoldmaker/meshtypes/meshtype_3d_brainstem.py Outdated Show resolved Hide resolved
src/scaffoldmaker/meshtypes/meshtype_3d_brainstem.py Outdated Show resolved Hide resolved
src/scaffoldmaker/meshtypes/meshtype_3d_brainstem.py Outdated Show resolved Hide resolved
src/scaffoldmaker/meshtypes/meshtype_3d_brainstem.py Outdated Show resolved Hide resolved
src/scaffoldmaker/annotation/brainstem_terms.py Outdated Show resolved Hide resolved
Comment on lines +217 to +229
eIndexPM['caudal-dorsal'] = int(elemPerLayer/2)
eIndexPM['midRostralCaudal-dorsal'] = int(elemPerLayer / 2) + (elemPerLayer * int((elementsCountAlong/2)-1))
eIndexPM['rostral-dorsal'] = (elemPerLayer*(elementsCountAlong-1)) + int(elemPerLayer/2)
eIndexPM['caudal-ventral'] = int(elemPerLayer/2) - (elementsCountAcrossMinor-1)
eIndexPM['midRostralCaudal-ventral'] = eIndexPM['midRostralCaudal-dorsal'] - (elementsCountAcrossMinor-1)
eIndexPM['rostral-ventral'] = int((elemPerLayer*(elementsCountAlong-1)) + (int(elemPerLayer/2) - elementsCountAcrossMinor + 1))
xiPM['caudal-ventral'] = [1.0, 0.0, 0.0]
xiPM['caudal-dorsal'] = [1.0, 0.0, 1.0]
xiPM['midRostralCaudal-ventral'] = [1.0, 1.0, 0.0]
xiPM['midRostralCaudal-dorsal'] = [1.0, 1.0, 1.0]
xiPM['rostral-ventral'] = [1.0, 1.0, 0.0]
xiPM['rostral-dorsal'] = [1.0, 1.0, 1.0]
for key in eIndexPM.keys():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're making the marker points as annotationGroups now. This makes the code cleaner as there isn't the risk of not matching by name, however it means we must request ILX terms from K-core; mainly requires putting the name and description on a Google spreadsheet.
These names are not sufficient however, as they must be qualified with 'Brainstem', 'midline' and 'point' to be fully prescriptive. Is 'rostral' or 'cranial' more appropriate here -- we need terms that make sense across species.
The names should be plain english, uncapitalised without camelCase.
I don't think we need triple qualified rostral+caudal+ventral/dorsal as only 2 should be sufficient (along with midline).
Wouldn't the midRostralCaudal* points be better on the medulla-pons boundary?
Alternatively to many of these points, we could define a midline group around the boundary...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am unsure what you want here. I think it's probably better to talk to the next mapper to make these changes.

Copy link
Member

@rchristie rchristie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fiducial marker points need to be redefined as annotation groups and qualified with brainstem and midline (or similar).

@rchristie rchristie merged commit 924fb6c into ABI-Software:master May 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants