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

document kim dev atlases in readme #444

Merged
merged 4 commits into from
Jan 23, 2025
Merged

document kim dev atlases in readme #444

merged 4 commits into from
Jan 23, 2025

Conversation

alessandrofelder
Copy link
Member

No description provided.

@alessandrofelder alessandrofelder marked this pull request as ready for review January 17, 2025 16:53
@adamltyson adamltyson marked this pull request as draft January 20, 2025 08:35
@adamltyson
Copy link
Member

I've put this back into draft status due to #441 (comment)

@PolarBean PolarBean mentioned this pull request Jan 23, 2025
7 tasks
@PolarBean
Copy link
Contributor

PolarBean commented Jan 23, 2025

hey I'm just trying to figure out how to update the README to include this update. The issue i'm having is up until now I could just list the API name of each atlas (ie: kim_dev_mouse_P04_LSFM_20um), but with this update there are now too many combinations to list all. instead of kim_dev_mouse_P04_LSFM_20um should I say kim_dev_mouse_{age}_{modality}_{resolution}um? thanks!

@carlocastoldi
Copy link
Contributor

instead of kim_dev_mouse_P04_LSFM_20um should I say kim_dev_mouse_{age}{modality}{resolution}um?

Just know that given a combination of (age, modality) there's only one resolution possible. Moreover, LSFM is always at 20um and, between between postnatals, it's always 50um.

Perhaps make the resolution semantically different, to signify that the user has no voice in deciding the resolution?
kim_dev_mouse_{age}_{modality}_<resolution>

@alessandrofelder
Copy link
Member Author

Sorry for the confusion @carlocastoldi and @PolarBean.
For clarification: Because of a fun capital letter related bug (#132) we can't have E, P, LSFM or MRI in the name (also good for consistency with other atlases).

There is a PR open to fix this #446, and I have regenerated the atlases with that script, so they have lowercase names now, as detailed in the PR too.

I will upload to the GIN data repo tomorrow (need to focus on other things today), and update the GIN readme and the docs PR brainglobe/brainglobe.github.io#271 accordingly :) please bear with me!

@alessandrofelder alessandrofelder marked this pull request as ready for review January 23, 2025 13:48
@alessandrofelder
Copy link
Member Author

This is ready to be re-reviewed now @adamltyson

(in respect to questions and suggestions above) I've put the atlases into the Readme separated by developmental timepoint, to be super-explicit about the names :) this is consistent with how it was for v0.0.1 which we had at P56 only.

@adamltyson adamltyson merged commit ae06859 into main Jan 23, 2025
14 checks passed
@adamltyson adamltyson deleted the update-kim-dev-docs branch January 23, 2025 16:31
@PolarBean
Copy link
Contributor

Thats a good solution for this atlas but for DeMBA (which has more than 50 ages) it may become a bit crowded.

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