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

Demba dev mouse #492

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Demba dev mouse #492

wants to merge 21 commits into from

Conversation

PolarBean
Copy link
Member

This is to integrate the DeMBA atlases into BrainGlobe. The main thing I would like input on at this stage is the README file. Since there are 53 days of atlases each with multiple modalities it makes the atlas api table huge. What do you think we should do? I could make a html table with filters that is hosted on the brainglobe info page instead of having the table in the README. What are your thoughts?

@PolarBean
Copy link
Member Author

I could also make some kind of api name wizard
image

@adamltyson
Copy link
Member

adamltyson commented Feb 5, 2025

That is a lot of atlases!

I'd rather not host an atlas-specific api name wizard if we can avoid it. Perhaps something simple like demba_p{AGE}_{MODALITY}_{RESOLUTION}um and then a list of available ages, modalities and resolutions?

This atlas could perhaps have a dedicated page on the website that could be linked to to outline this in more detail, and provide examples?

@PolarBean
Copy link
Member Author

Annoyingly not all resolutions are available for all modalities so the demba_p{AGE}_{MODALITY}_{RESOLUTION}um may not work. Sounds good to have a dedicated page, maybe this page can consist of just the different API names. This might also be a good solution for the kim atlas since it is taking up quite a lot of the README.

@adamltyson
Copy link
Member

Annoyingly not all resolutions are available for all modalities so the demba_p{AGE}{MODALITY}{RESOLUTION}um may not work.

But the available resolutions could be listed fairly succinctly right? I'm happy for the DeMBA to take up a few lines in the readme, just not 100's!

@alessandrofelder
Copy link
Member

not all resolutions are available for all modalities so the demba_p{AGE}{MODALITY}{RESOLUTION}um may not work.

Not a strong opinion, but I quite like the way the 38 kim_dev 1.0.0 ones are listed. (even though they take up a lot of space). Not all modalities exist for each developmental stage, so it's nice to have them listed explicitly in docs that are not meant to look as nice as our fancy webpage, and to avoid the (admittedly minimal) mental maths of "we have these 5 modalities at each developmental stage, and additionally this extra modality at E18.5 and P14 and P28" 🤷 again, not a strong opinion, happy either way

This atlas could perhaps have a dedicated page on the website that could be linked to to outline this in more detail, and provide examples?

FWIW I like this idea

@PolarBean
Copy link
Member Author

I have run the validator and it works. This has the same regions as the Allen for P56 and so it's appropriate to copy the Allen ontology. Younger brains should also have the same regions (it is possible that the warping shrunk some into non existence though I haven't check for this).

I will update the documentation and then finalise this draft pull request

image

@PolarBean
Copy link
Member Author

See the adjusted validation code attached here as TXT
validation.txt

@PolarBean
Copy link
Member Author

Ok I think we can start reviewing this and I'll update the documentation once we've decided how to format the table :)

@PolarBean PolarBean marked this pull request as ready for review February 7, 2025 16:43
@alessandrofelder
Copy link
Member

Thanks @PolarBean - I am leave most of next week, so review will have to wait until week of 17th, unfortunately.

@PolarBean
Copy link
Member Author

That's all good! Enjoy your leave 🏖️

@adamltyson
Copy link
Member

@carlocastoldi reminded me that atlases can have multiple reference images (brainglobe/brainglobe.github.io#281 (comment)).

For this atlas, would it be possible to resample the reference images and only create one packaged atlas per timepoint? One reference image could be picked as the "main" one, and the others added as additional references.

@carlocastoldi
Copy link
Contributor

carlocastoldi commented Feb 10, 2025

I am sorry if i sort of chime here as well, but I think the ~problems i know of Kim DevCCFv1 atlases are similar to those of DeMBA.

Namely:

  • the fact that at some agepoint, the total size of the reference images is small (a few hundreds MB, compressed) while at the adult stage it might be closer to be a 2+ GB, compressed. Should the use of "additional references" be restricted if the total to-be-downloaded size is too large? What if it's too large only at some, but not all, agepoints?
  • we shouldn't just choose a "main" reference modality for each atlas. I feel like we should rank the possible modalities (e.g. 1. Average > 2. Nissl > 3. LSFM > 4. MRI) and corresponding acronyms. When creating a BG atlas, then, we would pick the first available ranked modality as the "main", and the remaining as the "additional references". Ideally, this would allow brainglobe atlases, to be ~comparable, when possible, and use the same nomenclature

@PolarBean
Copy link
Member Author

I think it's a good idea to resample. But I wouldn't want to upsample only downsample. The reason is that the allen stpt is 10um resolution but the others are not.Iif I were to resample them up this would be misleading to the user as they would have no way of knowing which volumes were 'truely' a particular resolution.

In this case we would have three resolutions

25um
   * allen_stpt
   * mri
   * lsfm
   * stpt

20um
   * allen_stpt
   * stpt

10um
   * allen_stpt

@PolarBean
Copy link
Member Author

In regards to the atlases being too big with too many reference volumes. I think the real solution is to only download the reference templates when they are used, instead of downloading all for a particular age.

@PolarBean
Copy link
Member Author

but maybe that's for V2

@adamltyson
Copy link
Member

but maybe that's for V2

Yeah I think so, as it will require both changes to the packaging and the API

@alessandrofelder
Copy link
Member

Another limitation of the additional references is that they currently cannot be used as registration targets (IIUC, by the BrainGlobe tools). So for v1 I think we have to keep reference images that we want users to register to in separate atlases (and maybe prioritise moving to v2 rather than adding more v1 atlases altogether, including moving brainglobe-registration and brainreg to allow targetting any reference in the same atlas?)

@adamltyson
Copy link
Member

Another limitation of the additional references is that they currently cannot be used as registration targets (IIUC, by the BrainGlobe tools).

That's relatively easily fixed though. There are other considerations here, but we shouldn't let an issue with one tool that we have control over affect how we develop another.

@alessandrofelder
Copy link
Member

@PolarBean should I have a look at this yet, or would you like to implement the resampling first?

@PolarBean
Copy link
Member Author

It should be good. I am running the atlas generation overnight so if you wish I can notify you once that's done in the morning. Otherwise It's ready.

@alessandrofelder
Copy link
Member

Thanks, yea, let me know.
If you have the latest changes from main in this branch, you will have the latest validation functions all integrated into the wrapup function directly 🤞

Copy link
Member

@alessandrofelder alessandrofelder left a comment

Choose a reason for hiding this comment

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

I have had a look at the code, and it looks really nice!
I made some nitpicky comments that won't affect the functionality but IMO improve code legibility.

I am running this locally now too, and will approve when validation passes :)

@PolarBean
Copy link
Member Author

FYI the validation worked. See the updated validation script attached as a txt. validation.txt
image

@PolarBean
Copy link
Member Author

We have another version of this atlas which uses the kim developmental segmentations. I'm going to open a second pull request once this one is merged for it. Should I rename this to be demba_dev_allen_seg_mouse and have that one as demba_dev_kim_seg_mouse ?

@adamltyson
Copy link
Member

Should I rename this to be demba_dev_allen_seg_mouse and have that one as demba_dev_kim_seg_mouse ?

Sounds good to me.

@alessandrofelder
Copy link
Member

Happy with suggested names too

@PolarBean would you be able to share the tar.gz files directly when you're done?
Maybe even including the renaming?
Seems more efficient for this scale of atlas packaging than me regenerating them locally (twice) 😁

@PolarBean
Copy link
Member Author

Yeah I can do that :)

@alessandrofelder
Copy link
Member

(I can merge this and wait for your follow-up renaming PR if that's helpful?)

@PolarBean
Copy link
Member Author

How about we do the renaming before merging. I like the idea of rarely renaming atlases once they are merged.

@alessandrofelder alessandrofelder self-requested a review February 19, 2025 15:53
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.

4 participants