-
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
Perens stereotaxic mri #370
base: main
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
brainglobe_atlasapi/atlas_generation/atlas_scripts/perens_stereotaxic_mri_mouse.py
Outdated
Show resolved
Hide resolved
…eotaxic_mri_mouse.py Co-authored-by: Adam Tyson <[email protected]>
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 a lot for this @PolarBean
I've left some high-level comments for now - let me know what you think.
brainglobe_atlasapi/atlas_generation/atlas_scripts/perens_stereotaxic_mri_mouse.py
Outdated
Show resolved
Hide resolved
brainglobe_atlasapi/atlas_generation/atlas_scripts/perens_stereotaxic_mri_mouse.py
Outdated
Show resolved
Hide resolved
brainglobe_atlasapi/atlas_generation/atlas_scripts/perens_stereotaxic_mri_mouse.py
Outdated
Show resolved
Hide resolved
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
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.
Looking good - thanks @PolarBean
Some more very minor comments.
brainglobe_atlasapi/atlas_generation/atlas_scripts/perens_stereotaxic_mri_mouse.py
Outdated
Show resolved
Hide resolved
brainglobe_atlasapi/atlas_generation/atlas_scripts/perens_stereotaxic_mri_mouse.py
Outdated
Show resolved
Hide resolved
brainglobe_atlasapi/atlas_generation/atlas_scripts/perens_stereotaxic_mri_mouse.py
Outdated
Show resolved
Hide resolved
brainglobe_atlasapi/atlas_generation/atlas_scripts/perens_stereotaxic_mri_mouse.py
Outdated
Show resolved
Hide resolved
brainglobe_atlasapi/atlas_generation/atlas_scripts/perens_stereotaxic_mri_mouse.py
Outdated
Show resolved
Hide resolved
brainglobe_atlasapi/atlas_generation/atlas_scripts/perens_stereotaxic_mri_mouse.py
Outdated
Show resolved
Hide resolved
…eotaxic_mri_mouse.py Co-authored-by: Alessandro Felder <[email protected]>
…eotaxic_mri_mouse.py Co-authored-by: Alessandro Felder <[email protected]>
@alessandrofelder reformatted to the new template and addressed your comments. Feel free to re-review when you get a chance. |
brainglobe_atlasapi/atlas_generation/atlas_scripts/perens_stereotaxic_mri_mouse.py
Show resolved
Hide resolved
@PolarBean is it possible to add a few more print statements, and ideally a download progress bar? It's hard to know what's going on when running this script. |
Thanks for working on this @PolarBean. I'm not exactly sure what files are packaged with this atlas, but presumably we should be packaging up multiple reference (template) images? If I understand correctly, the main reason to add this atlas would be to make available all the different reference images as registration targets? It may also be worth packaging up multiple atlases, one for each coordinate space. |
This atlas only has one template image (MRI). It is in a different space to the perens LSFM atlas. It's valuable because it is in flat skull position and is therefore useful for stereotaxic surgery. It is also useful as a MRI registration target. I will make a seperate pull request for the most recent perens lsfm volume once they have updated those files on their server. |
Ah, nvm. I assumed as it was a "multimodal" atlas framework, there were templates for LSFM & CT in the same space. |
Yeah they have multiple templates in multiple spaces and they are linked by deformation matrices. |
Also one thing I'm curious about is what should our approach be to adding new resources to brainglobe. Should a new atlas provide some benefit in order to be added, or instead is just the existence of a resource enough to add it (even if it has been superseded by newer better resources)? I would tend towards wanting to add in everything, as it could be useful for say replicating past results or comparing atlas definitions of particular regions. But others may disagree. |
I agree with you. As an example this atlas was added to allow for some pre-BrainGlobe analyses to be replicated with BrainGlobe tools. If an atlas has obviously been replaced by another, then I would suggest we:
|
No worries, I've added a download progress bar and a few more print statements. Let me know if it's what you had in mind |
Yes, thanks! A bit more printing (e..g just to say what's being downloaded might be useful too). I haven't checked the packaged atlas in detail (I'll leave that to @alessandrofelder when he's back), but I noticed a couple of issues:
|
I see, I'll have to recheck it. I thought it was unchanged from the previous perens lsfm in the apiwhich is listed as IAL and so I copied it from there. I will recheck it for this one and for the other pull requests I have. |
Hmm, I contacted Gubra about some problems I was facing when ingesting the LSFM volume (for which I will soon open a pull request). They fixed that problem and it seems they also updated the MRI annotation while they were at it. When I run this script I now get 838 meshes which seems much more respectable! |
I read the documentation but Im a bit confused about differentiating between ars and als for symmetrical atlases. I put als as i thought left to right seemed most intuitive but I am a bit lost. |
In principle that shouldn't make a difference. |
it actually only downloads a single file as all the atlas data is distributed in a single archive. I'll add print statements elsewhere though |
ok actually i added one more print statement but it seems now every function has at least one print statement. I think I've addressed everything now but feel free to add more recommendations. I'll also update the template script with print statements so it should be included by default in the future. |
I ran the validation script Alessandro sent me and it returned the following error
I'll try to fix the xcb error but the missing structures are likely just due to those regions being too small in this atlas. |
Thanks @PolarBean - I'll review next week! |
Here I added the Perens MRI stereotaxic atlas
Description
What is this PR
References
#203
How has this PR been tested?
I ran the atlas generation script, and inspected the output.
I attempted to run the mesh checking however I am missing the correct drivers for libGL.