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

Add DevCCFv1 LSFM@20µm resolution #441

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

Conversation

carlocastoldi
Copy link

Description

With this draft PR i ask you your opinion and help to first:

  • allow the script to download the resources from the official OneDrive page
  • have a general review of the script's code quality before adapting it to generate the other combination of atlases/references
  • choose which atlases are worth generating and whether they should update any previous atlas

Lastly, I have tested this script on two setups: on one computer without parallelization and the other with. The run with parallelization didn't manage the memory properly and run OOM on linux, effectively making the generation of the atlas much slower. Does BrainGlobe offer any help in the management of the parallelization or should I do it myself?

P.S. if needed, I can provide the resulting atlas from this script

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?
To perfection the support to Kim's developing mouse brain atlases

What does this PR do?
It adds a script for generating Kim's DevCCFv1 mouse atlas. For now, it generates only the P14 altas with LSFM@20µm as reference, ignoring the other ages (E11.5, E13.5, E15.5, E18.5, P4 and P56) and references (they are all MRI@50 µm)

References

#425

How has this PR been tested?

The resulting atlas from the script was used within ABBA

Is this a breaking change?

No.

Does this PR require an update to the documentation?

It does, but it is not included yet in this PR

Checklist:

  • The code has been tested locally
  • Tests have been added to cover all new functionality (unit & integration)
  • The documentation has been updated to reflect any changes
  • The code has been formatted with pre-commit

@alessandrofelder
Copy link
Member

Hey @carlocastoldi

Thanks a lot!

Does BrainGlobe offer any help in the management of the parallelization or should I do it myself?

We are aware that past parallelisation efforts are not robust #428, so if you would like to contribute a better way, even in just for one atlas, that might help us address #429 - so that would be great

choose which atlases are worth generating and whether they should update any previous atlas

I am happy to give my thoughts on code quality and how to download from OneDrive but can I first check (Pardon my naivety!): what is the difference here to the existing Kim Developmental atlas that is already packaged? Could this be an extension of that existing script?

@adamltyson
Copy link
Member

what is the difference here to the existing Kim Developmental atlas that is already packaged? Could this be an extension of that existing script?

I think that is an earlier version of the P56 atlas.

@carlocastoldi ideally we would add all these atlases, but the number of ages x reference images is very large. In principle though, there's no reason why we can't add them all. Each would have to be a seperate atlas (at least until we built Atlas API V2). Would you be willing to write a script that can add all of these atlases?

@carlocastoldi
Copy link
Author

We are aware that past parallelisation efforts are not robust #428, so if you would like to contribute a better way, even in just for one atlas, that might help us address #429 - so that would be great

Oh okay, i see that! It's something I may perhaps look into, but for now it can't be my priority...

can I first check (Pardon my naivety!): what is the difference here to the existing Kim Developmental atlas that is already packaged? Could this be an extension of that existing script?

Oh I tried to understand if the existing "developmental" atlas (it's not really developmental because it's only for P56, when mice are already adult. It's the same age at which Allen developed CCFv3) had any publication associated with it, but as far as i understood, that's only a v0.0.1 of the work they have recently published (v1). I am not 100% sure about it, tho, as i can't find any description/publication about that dataset.
Surely, however, it is not compatible with their v1 as they differ in resolution, coordinate system, ontology and reference.
Rather, these atlases are closer to those of ADMBA, however they would also make them outdated (ADMBA's are very low resolution and hard to work with due to their undefined region boundaries).

Lastly they offer two different resolutions: one at 20µm for LSFM references and one at 31.5µm/40µm/50µm (depending on the age of the atlas) for MRI references and some stereotaxic coordinates (for post-natal atlases).
For now, in this PR, i would focus on adding just LFSM@20µm atlases.

I am happy to give my thoughts on code quality and how to download from OneDrive

Thank you a lot!

@adamltyson
Copy link
Member

For now, in this PR, i would focus on adding just LFSM@20µm atlases.

That seems sensible, thanks!

@carlocastoldi
Copy link
Author

@carlocastoldi Would you be willing to write a script that can add all of these atlases?

Oh i missed this message, sorry!
But yes, sure. I'm down for it. However at the moment the script assumes that the resources are already downloaded and it does not retrieve them from official channels as they are on OneDrive

@carlocastoldi
Copy link
Author

only one question remains: would you be okay if this PR also deleted the scripts for generating ADMBA and old KimP56 atlases?

They are all superseded by this new version for the developing mouse. Leaving them all would otherwise overcrowd the list of supported atlases

@adamltyson
Copy link
Member

For now, I think we should keep them. Although the new atlases may be better, there are still researchers using the old ones for ongoing projects.

We also want to ensure that BrainGlobe acts as an archive in case the original files cannot be found for older atlases.

Longer term, we will find a solution for making the set of available atlases easier to navigate (e.g. atlas groups).

@carlocastoldi
Copy link
Author

Sure
Thank you for the explanation!

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.

Thanks again for this, @carlocastoldi - and thank you and @adamltyson for your clarifications.

The code looks largely good on a first pass (I haven't run it, but it's promising you could use it in ABBA 🙌 ), made two small comments.

Do either of you know how official the OneDrive folder is? It looks like in the publication they point to this Figshare repo. Using this repo instead would make it easy to use pooch to download and cache them neatly (and it's likely a more robust place to get data from long-term)?

PACKAGER = "Carlo Castoldi <castoldi[at]ipmc.cnrs.fr>"
ATLAS_FILE_URL = "https://pennstateoffice365-my.sharepoint.com/personal/yuk17_psu_edu/_layouts/15/download.aspx?UniqueId=fe3d1692%2D94e4%2D4238%2Db6bc%2D95d18bcac022"
# curl 'https://pennstateoffice365-my.sharepoint.com/personal/yuk17_psu_edu/_layouts/15/download.aspx?UniqueId=fe3d1692%2D94e4%2D4238%2Db6bc%2D95d18bcac022' -H 'User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:132.0) Gecko/20100101 Firefox/132.0' -H 'Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8' -H 'Accept-Language: en-GB,en;q=0.5' -H 'Accept-Encoding: gzip, deflate, br, zstd' -H 'Referer: https://pennstateoffice365-my.sharepoint.com/personal/yuk17_psu_edu/_layouts/15/onedrive.aspx?id=%2Fpersonal%2Fyuk17%5Fpsu%5Fedu%2FDocuments%2FProjects%2FDevelopmental%5FAtlasing%2FDevCCF%5FTeamFileshare%2FDevCCF%5FTemplates%2FDevCCFv1%2B%2FDevCCFv1' -H 'Upgrade-Insecure-Requests: 1' -H 'Sec-Fetch-Dest: iframe' -H 'Sec-Fetch-Mode: navigate' -H 'Sec-Fetch-Site: same-origin' -H 'Connection: keep-alive' -H 'Cookie: FedAuth=77u/PD94bWwgdmVyc2lvbj0iMS4wIiBlbmNvZGluZz0idXRmLTgiPz48U1A+VjEzLDBoLmZ8bWVtYmVyc2hpcHx1cm4lM2FzcG8lM2Fhbm9uIzc1Yjc1NjA0NGUxZDI5ZDU5NjFmMjZjODc0MzFlZmQwNmU4ZWM2NzYwMTJmZGEyNjgxYzg4Zjc5ODdmMGY2YTAsMCMuZnxtZW1iZXJzaGlwfHVybiUzYXNwbyUzYWFub24jNzViNzU2MDQ0ZTFkMjlkNTk2MWYyNmM4NzQzMWVmZDA2ZThlYzY3NjAxMmZkYTI2ODFjODhmNzk4N2YwZjZhMCwxMzM3NjQ5NTA5NzAwMDAwMDAsMCwxMzM3NjY3MzA4NzYxOTQyMTMsMC4wLjAuMCwyNTgsN2NmNDhkNDUtM2RkYi00Mzg5LWE5YzEtYzExNTUyNmViNTJlLCwsZWU2YTY1YTEtMDBmNS02MDAwLWZiMDYtYTgzYjk1Yjg3NTIzLDkxYzI2NWExLTMwNDctNjAwMC1mYjA2LWFiY2IwMTQ2MTZmNSxDR1EvcVlGcGJFeVlJTHAvM2NHb3pnLDAsMCwwLCwsLDI2NTA0Njc3NDM5OTk5OTk5OTksMCwsLCwsLCwwLCwxOTI2MDksdVhlaFFKUGxlVmpOQ2Jha1VoR0Q2SXlGUVFrLFkzdXYyY1dWdUZMWElneHRSOW4vMVVValJ1UUNMd2wybzdZbE51S0RtNTBxOE16ODJVaCt6K0ZTL0NqUU0rNlZrc090RlkwY211OUJNWHllc2U3Z3dST1VMcXRDWEorV3M1NkVzcEZrdk1qTEI0VzNhbDVZeVRlZ0greURRaUFRbG8rVG82aUVmVndVODU5Z2RWME5GSUprenJSVUlCeWVDUThiMmhMRjhQNmlJTDZHYldBNXVwRnJpcTl6cml6THhheFBRUWtVRzRoUmJjMEgzaVMzMnVKekw1ckFuWnVZSFk1N2J4K2xZOGE5MUNVQ3BSQXJ3aU45NHZ5aXdaQWdlcDRXUkxwWlVWOU5NNFZwZ0xUN05zVUh2K0JBcGwxbnViY0FDVENlWElhbWNwclZ0eTBFdElpajVSeXR3UGk0OFRlSVlIQXVrRjhDUFRFVEdDcTlGQT09PC9TUD4=; FeatureOverrides_experiments=[]; MicrosoftApplicationsTelemetryDeviceId=8293cff3-d815-447c-b343-546e678a0450; MSFPC=GUID=79c2671112584c09b960ec538dca634d&HASH=79c2&LV=202411&V=4&LU=1732022021463; ai_session=O/d7EbWPiaNaTZQh0qUYpP|1732113248412|1732113248412' --output P14.zip
PARALLEL = False # disable parallel mesh extraction for easier debugging
Copy link
Member

Choose a reason for hiding this comment

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

As discussed in #441 (comment), for now, I'd either

Comment on lines 217 to 219
atlas_root = "/home/castoldi/Downloads/DevCCFv1"
# Generated atlas path:
DEFAULT_WORKDIR = Path.home() / "brainglobe_workingdir"
Copy link
Member

Choose a reason for hiding this comment

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

Once we figure out how to download the data, we should avoid this hard-coded path (and ideally replace it with a pooch temporary file)

Also, you can import the DEFAULT_WORKDIR

from brainglobe_atlasapi.config import DEFAULT_WORKDIR

(so we can change it with one-line in the future!)

@carlocastoldi
Copy link
Author

Do either of you know how official the OneDrive folder is?

Is the official download page from their website, apparently.

One that could get updated and be changed when new versions come out, I imagine.

@alessandrofelder
Copy link
Member

alessandrofelder commented Dec 20, 2024

Hmmm, I see.

I'd still prefer going with the figshare data if possible, because

  • it has a license
  • it is versioned by a DOI
  • it is linked to from the publication we include in the metadata
  • (it's easier to download programmatically... haven't found a way to easily generate a sensible download link from OneDrive)

I'm not sure how standardised new versions published on OneDrive would be?
Would they make a new folder somewhere? Would they overwrite some of the existing files?
I may be missing something.

What do you think @carlocastoldi ?

@carlocastoldi
Copy link
Author

Sure, i see all the upsides of using figshare. I guess I was worried that we wouldn't notice any update to the published atlas unless we actively search for them, but that's a minor issue!

I'll work on this PR by using pooch on Figshare. Thank you all!

@carlocastoldi carlocastoldi marked this pull request as ready for review December 20, 2024 19:53
@carlocastoldi
Copy link
Author

With this last commit, i think this PR is ready for review!

@carlocastoldi carlocastoldi changed the title Add DevCCFv1 P14 LSFM@20µm resolution Add DevCCFv1 LSFM@20µm resolution Dec 20, 2024
@adamltyson
Copy link
Member

Thanks @carlocastoldi. The BrainGlobe team is checked out for the holidays now, so we'll take a look in the new year.

@carlocastoldi
Copy link
Author

I figured!
I just wanted to have my part done before holidays. No hurry!

Happy holidays! ^^

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.

3 participants