Allen bbp ccfv3a entire mouse brain, improve mesh utils, and add metadata filtering to wrapup #500
Conversation
|
given that this relies on changes to mesh utils you need to |
|
Found an error. Closed until I fix |
|
Ok fixed some orientation problems, but another issue was the datatype. It seems all brainglobe atlases must be 16 bit unsigned integers. Is this in the integration docs? definitely something to add, not sure where though. Ive also started using dataclasses here, I was thinking about upstreaming them into brainglobe and importing the atlas_metadata class in future atlas requests. Theres this dictionary here that could be replaced by a dataclass if youd like and imported as needed https://github.com/brainglobe/brainglobe-atlasapi/blob/main/brainglobe_atlasapi/descriptors.py |
Don't think it's documented anywhere. It should probably go somewhere here. |
There was a problem hiding this comment.
Thanks a lot @PolarBean! Generally, looks very nice, in particular the additional helper functions 🙌
Could you remove left-over docstrings from the template script, and replace them with descriptions of this atlases idiosyncrasies (many of which you already detail in code comments), please?
Then I can run and validate locally too :)
brainglobe_atlasapi/atlas_generation/atlas_scripts/allen_bbp_ccfv3a_entire_mouse_brain.py
Outdated
Show resolved
Hide resolved
| METADATA = AtlasMetadata( | ||
| version=0, | ||
| name="allen_bbp_ccfv3a_entire_mouse_brain", | ||
| citation="https://doi.org/10.1101/2024.11.06.622212", | ||
| atlas_link="https://zenodo.org/api/records/14034334/files-archive", | ||
| species="Mus musculus", | ||
| orientation="spr", | ||
| root_id=997, | ||
| resolution=25, | ||
| ) |
There was a problem hiding this comment.
Not a strong opinion, but maybe a worry would be that using a dataclass (while nice syntactic sugar for more experienced developers) introduces a small barrier for novice programmers to contribute atlases in the future (because they need to understand what e.g. METADATA.name refers to)?
There was a problem hiding this comment.
Interesting point, I don't think its so difficult for novices but its hard to say. I think the main point of this change is to organise all the metadata in one place with detailed descriptions of what each field requires and what type of data it expects. I think doing this with constants and comments in a template file is less than ideal perhaps but I'm open to opinions.
There was a problem hiding this comment.
yea, it's a trade-off. Not sure - will bring up at dev meeting tomorrow.
There was a problem hiding this comment.
I would vote against dataclasses. Atlas contributors tend to be the least experienced contributors we have (some have no Python experience). Anything we can do to simplify this process is good.
There was a problem hiding this comment.
fair point! should I remove it from this script or is it ok
There was a problem hiding this comment.
Let's remove it for consistency across packaging scripts?
brainglobe_atlasapi/atlas_generation/atlas_scripts/allen_bbp_ccfv3a_entire_mouse_brain.py
Show resolved
Hide resolved
| closing_n_iters = 2 | ||
| decimate_fraction = 0 | ||
| smooth = False |
There was a problem hiding this comment.
I wonder whether these should be exposed as parameters, with default values, so we can re-use this function across more packaging scripts (as and when we need to, moving to version 2)
There was a problem hiding this comment.
I exposed these parameters. I havent tested it yet as im currently generating demba files (and will be for a while!). There is the option to expose all of the parameters of extract mesh from mask but I'm not to familiar with that function.
brainglobe_atlasapi/atlas_generation/atlas_scripts/allen_bbp_ccfv3a_entire_mouse_brain.py
Show resolved
Hide resolved
…cfv3a_entire_mouse_brain.py Co-authored-by: Alessandro Felder <[email protected]>
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
We should also think about testing the helper functions (here or in a different PR) |
|
hmm unless the other helper functions are currently covered by tests I would suggest doing that in another PR |
I agree - would you be happy to take that on? |
|
I don't think I have the bandwidth at the moment aha sorry |
|
OK! Tracked in #503 |
…ren in segmentation
|
This is ready for review :) |
There was a problem hiding this comment.
At least for the versions I generated locally:
It looks like there's a mismatch in the mesh dimensions in the the 10um version?
which is also flagged by the validation:
Failed validations:
('validate_mesh_matches_image_extents', "('Mesh coordinate 4985.0 and annotation coordinate 0.0', 'differ by more than 10 times pixel size 10.0')")The dimensions match for 25um,
and it looks to me like the 10um version is not scaled - I don't see from the code why that happens though. Can you reproduce @PolarBean ?
There also seem to be some extra meshes?
('catch_missing_structures', 'Structures with IDs [991, 254, 909, 567, 714, 669, 453, 792, 1008, 528, 500, 95, 315, 247, 22, 322, 31] have a mesh file, but are not accessible through the atlas.')These are presumably expected, as in the Allen atlas, e.g #109
('catch_missing_mesh_files', "Structures with IDs [545, 1051] are in the atlas, but don't have a corresponding mesh file.")It also looks like the resulting BrainGlobe orientation is not the expected ASR (should open as a coronal section in napari by default).
|
Yeah the meshes not being scaled is because I tried saving time using the 25um meshes for both. I don't know why but I thought brainglobe atlases did that. It might be what we do internally and I had crossed wires. I thought I had fixed the issue with removing too many regions from the metadata.... let me work on that and then rerun the validation.... |
|
It was only checking if an id or its children were in the annotation, I needed to check whether an id or any of its descendants were in the annotationand only if not then discard it from the metadata. There were some regions that werent in the annotationbut had grandchildren and great grandchildren in the annotation. |
Here I add the extended and improved CCFv3 annotation and Nissl atlas of the entire mouse brain by Piluso and colleagues (full disclosure I'm also an author).
I have also integrated the filtering script we have previously talked about that automatically removes metadata regions that do not exist in the annotation, previously a point of friction in integration scripts. I moved int into the wrapup script.
Another addition here is the addition of a annotation + region dictionary to mesh function. This code is the same in every script I submit and is a large effectively boiler plate function that is copy and pasted between them. By moving it into mesh utils we make atlas integration easier and the integration scripts cleaner.