-
Notifications
You must be signed in to change notification settings - Fork 33
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
Atlas packaging template #400
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
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.
Thanks a lot for this @PolarBean - looking good.
I've made many comments, but most of them are small. I've tried to reduce duplicate info by making the example script doc-strings cater to people that like concrete examples, and make the template script doc-strings more instructions about what one needs to do.
I still kind of think we could do with just one and not the other, but happy to keep both for now. We can think more about this as we refactor the API and head towards version 2.
As always, I am open to counter-arguments to my suggestions 🙂
(I've made some suggestions to make the linter happier with the template script, but it's unlikely to have covered all of it. That also needs to be addressed before we merge this.)
Some questions I have:
- should we explain somewhere how to include additional references or is that considered out-of-scope here?
- should we point to mesh helper functions somewhere?
brainglobe_atlasapi/atlas_generation/atlas_scripts/example_mouse.py
Outdated
Show resolved
Hide resolved
brainglobe_atlasapi/atlas_generation/atlas_scripts/template_script.py
Outdated
Show resolved
Hide resolved
brainglobe_atlasapi/atlas_generation/atlas_scripts/template_script.py
Outdated
Show resolved
Hide resolved
brainglobe_atlasapi/atlas_generation/atlas_scripts/template_script.py
Outdated
Show resolved
Hide resolved
brainglobe_atlasapi/atlas_generation/atlas_scripts/template_script.py
Outdated
Show resolved
Hide resolved
brainglobe_atlasapi/atlas_generation/atlas_scripts/example_mouse.py
Outdated
Show resolved
Hide resolved
brainglobe_atlasapi/atlas_generation/atlas_scripts/template_script.py
Outdated
Show resolved
Hide resolved
brainglobe_atlasapi/atlas_generation/atlas_scripts/template_script.py
Outdated
Show resolved
Hide resolved
Should we have an explicit section like: # **** OPTIONAL *****
additional reference stuff |
…se.py Co-authored-by: Alessandro Felder <alessandrofelder@users.noreply.github.com>
…ript.py Co-authored-by: Alessandro Felder <alessandrofelder@users.noreply.github.com>
…ript.py Co-authored-by: Alessandro Felder <alessandrofelder@users.noreply.github.com>
…ript.py Co-authored-by: Alessandro Felder <alessandrofelder@users.noreply.github.com>
…se.py Co-authored-by: Alessandro Felder <alessandrofelder@users.noreply.github.com>
…se.py Co-authored-by: Alessandro Felder <alessandrofelder@users.noreply.github.com>
for more information, see https://pre-commit.ci
…ript.py Co-authored-by: Alessandro Felder <alessandrofelder@users.noreply.github.com>
…ript.py Co-authored-by: Alessandro Felder <alessandrofelder@users.noreply.github.com>
…ript.py Co-authored-by: Alessandro Felder <alessandrofelder@users.noreply.github.com>
…ript.py Co-authored-by: Alessandro Felder <alessandrofelder@users.noreply.github.com>
for more information, see https://pre-commit.ci
…se.py Co-authored-by: Alessandro Felder <alessandrofelder@users.noreply.github.com>
…se.py Co-authored-by: Alessandro Felder <alessandrofelder@users.noreply.github.com>
…se.py Co-authored-by: Alessandro Felder <alessandrofelder@users.noreply.github.com>
…ript.py Co-authored-by: Alessandro Felder <alessandrofelder@users.noreply.github.com>
this seems like a good solution but i cant see any instances of an atlas that has multiple citable documents (if this is what you're referring to). How should this be implemented? |
@alessandrofelder I've addressed your comments and am ready for this to be re-reviewed once you have the time :) |
Here we mean multiple reference images (i.e. templates, we should rename this!). I think the AZBA has 2 reference images. |
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 @PolarBean - looking good!
Two more tiny comments, then we can merge this.
brainglobe_atlasapi/atlas_generation/atlas_scripts/example_mouse.py
Outdated
Show resolved
Hide resolved
expected_annotation_hash = ( | ||
"451e6a82f531d3db4b58056d024d3e2311703c2adc15cefa75e0268e7f0e69a4" | ||
) | ||
reference_hash = pooch.file_hash( |
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.
I worry that this is
a) not the way we typically use pooch
and therefore confusing
b) more importantly, might look scary to new contributors coming more from the biology side
I'm tempted to remove any pooch
-related things, and mention it in the template script only (which we already do in this PR)?
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.
I see your point, the only thing is that pooch here actually serves a purpose, to ensure that the files from the Allen don't change without notifying us. In this instance we would want pooch to confirm the hash of the file. I understand that it's non standard but I think that just comes with using the Allen API in the example.
An alternative would be to not use the allen API at all, instead just use the files from the allen informatics archive. The advantage to this is the example would become more of a standard atlas integration allowing us to use pooch, etc, instead of so much allen specific logic.
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.
An alternative would be to not use the allen API at all, instead just use the files from the allen informatics archive.
I think this would be great if you have the patience to implement this? 😁 Would this be possible/easy on the OntologiesAPI
side for the meshes too?
The alternative is that we make a new issue to at some point move to a more standard atlas (e.g. we have recently generated a bird template for which we will host the data ourselves, and which has ITK snap annotations - our standard - so we could use that when the preprint is published.)
…se.py Co-authored-by: Alessandro Felder <alessandrofelder@users.noreply.github.com>
Following on from the discussion at #399 I have created a functional version of the template script and updated example mouse to follow the new template.