-
Notifications
You must be signed in to change notification settings - Fork 34
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
added nadkarni_mri_mouselemur #411
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @PolarBean
Largely looks good to me - made some comments to increase the code quality (I hope!)
I've run the validation on the atlas locally, and it looks like there's some spurious pixels in the annotation here (see left of canvas in screenshot below):
Not sure whether we should ask the original authors to fix this, or fix it ourselves programmatically. Thoughts welcome!
import time | ||
from pathlib import Path | ||
|
||
import mp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC
- we want to remove this way of parallelising mesh creation because it has caused us some problems?
- if we did want to keep it, this should be
import multiprocessing as mp
?
# The id of the highest level of the atlas. This is commonly called root or | ||
# brain. Include some information on what to do if your atlas is not | ||
# hierarchical |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest that in general we remove copy-pasted comments and docstrings from the template script (or update them to make them more concrete and specific to this atlas), to aid legibility?
new_df = pd.DataFrame() | ||
new_df["id"] = df["IDX"] | ||
new_df["name"] = df["LABEL"] | ||
new_df["acronym"] = df["LABEL"] | ||
new_df["rgb_triplet"] = df[["R", "G", "B"]].values.tolist() | ||
new_df["structure_id_path"] = new_df["id"].apply(lambda x: [ROOT_ID, x]) | ||
new_df["rgb_triplet"] = df[["R", "G", "B"]].values.tolist() | ||
root = pd.DataFrame( | ||
{ | ||
"id": [ROOT_ID], | ||
"name": ["root"], | ||
"acronym": ["root"], | ||
"structure_id_path": [[ROOT_ID]], | ||
"rgb_triplet": [[255, 255, 255]], | ||
} | ||
) | ||
new_df = pd.concat([root, new_df]).reset_index(drop=True) | ||
new_df = new_df[new_df["name"] != "Clear Label"].reset_index(drop=True) | ||
return new_df.to_dict("records") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lovely!
Tangential comment: Do I understand correctly that this function processes a ITKsnap output segmentation?
If so (in a separate issue) we should make this a helper function (because ITK snap is our default for creating new annotations).
meshes_dir_path = download_dir_path / "meshes" | ||
meshes_dir_path.mkdir(exist_ok=True) | ||
|
||
tree = get_structures_tree(structures) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we pass structures
as an argument to this function (because otherwise we rely on structures
existing in the global namespace, which is brittle?)
|
||
smooth = False | ||
start = time.time() | ||
if PARALLEL: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned before, I suggest we remove the parallel if clause here.
Here this addresses this issue. Adding a mouse lemur atlas into brainglobe.
#410
This will be ready once I double check the orientation.