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

Standardising atlas file downloading #351

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

Conversation

viktorpm
Copy link
Contributor

@viktorpm viktorpm commented Aug 9, 2024

Description

This PR introduces a standardized method for downloading atlas files using pooch.

  • Bug fix
  • Addition of a new feature
  • Other

How has this PR been tested?

Scripts were tested locally

Is this a breaking change?

No

Does this PR require an update to the documentation?

Potentially

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

@viktorpm viktorpm marked this pull request as ready for review September 4, 2024 10:50
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 a lot @viktorpm - this is good progress!
And sorry for letting this sit so long 🙈

I've suggested some minor changes and asked for minor clarifications.

Some high-level comments

  • there's quite a bit of commented out code that I think should be removed
  • I'm confused by the ### comments, but if we want to keep them we should add them everywhere.

We should open issues for

  • removing the current way of parallelising mesh creation everywhere
  • decide what to do with __version__ and __atlas__
  • make mesh creation parallel in a more robust way

What do you think?

@@ -1,52 +1,58 @@
__version__ = "1"

### Import
Copy link
Member

Choose a reason for hiding this comment

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

Are these comments useful and why do they start with ###? I may be missing something. If they are useful, should they be added to the admba_3d_dev_mouse script too?

Comment on lines +221 to +231
# ATLAS_NAME = "allen_cord"
# SPECIES = "Mus musculus"
# ATLAS_LINK = "https://data.mendeley.com/datasets/4rrggzv5d5/1"
# CITATION = (
# "Fiederling et al. 2021, https://doi.org/10.1101/2021.05.06.443008"
# )
# ORIENTATION = "asr"
# RESOLUTION = (20, 10, 10)
# ROOT_ID = 250
# ATLAS_FILE_URL = "https://prod-dcd-datasets-cache-zipfiles.s3.eu-west-1.amazonaws.com/4rrggzv5d5-1.zip"
# ATLAS_PACKAGER = "MetaCell LLC, Ltd."
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# ATLAS_NAME = "allen_cord"
# SPECIES = "Mus musculus"
# ATLAS_LINK = "https://data.mendeley.com/datasets/4rrggzv5d5/1"
# CITATION = (
# "Fiederling et al. 2021, https://doi.org/10.1101/2021.05.06.443008"
# )
# ORIENTATION = "asr"
# RESOLUTION = (20, 10, 10)
# ROOT_ID = 250
# ATLAS_FILE_URL = "https://prod-dcd-datasets-cache-zipfiles.s3.eu-west-1.amazonaws.com/4rrggzv5d5-1.zip"
# ATLAS_PACKAGER = "MetaCell LLC, Ltd."

I think this can be removed instead of just commenting it out.


working_dir = working_dir / ATLAS_NAME

working_dir = working_dir
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
working_dir = working_dir

I think this line is superfluous?

ORIENTATION = "rpi"

### Settings
PARALLEL = True # 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.

Does parallelisation work for this atlas? It's probably not good to have it as a default if not... and recently we've been moving away from parallelisation here?

@@ -251,6 +255,8 @@ def prune_tree(tree):
total=tree.size(),
description="Creating meshes",
):

# root_node = tree.nodes[ROOT_ID]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# root_node = tree.nodes[ROOT_ID]

We should remove unused code.

@@ -1,9 +1,9 @@
__version__ = "1"
Copy link
Member

Choose a reason for hiding this comment

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

We should be consistent if we want to move the version away from here (but I'm not sure we do?)

# Temporary folder for download:
download_dir_path = working_dir / "downloads"
download_dir_path.mkdir(exist_ok=True)
atlas_files_dir = download_dir_path / "atlas_files"
# atlas_files_dir = download_dir_path / "atlas_files"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# atlas_files_dir = download_dir_path / "atlas_files"

Remove unused code.

ATLAS_FILE_URL = (
"https://github.com/Gubra-ApS/LSFM-mouse-brain-atlas/archive/master.tar.gz"
)
RESOLUTION = 20 # some resolution, in microns
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the comment - my best guess is that it's a relic from someone explaining the template script to themselves? If you agree, @viktorpm let's remove (if not, please clarify the comment)

# Temporary folder for download:
download_dir_path = working_dir / "downloads"
download_dir_path.mkdir(exist_ok=True)
atlas_files_dir = download_dir_path / "atlas_files"
# atlas_files_dir = download_dir_path / "atlas_files"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# atlas_files_dir = download_dir_path / "atlas_files"

Remove unused code

@@ -217,7 +221,7 @@ def create_atlas(working_dir):
), "No download link provided for atlas in ATLAS_FILE_URL"

# Generated atlas path:
working_dir = working_dir / ATLAS_NAME
working_dir = working_dir
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
working_dir = working_dir

I think this line is superfluous?

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.

2 participants