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

Atlas packaging template #400

Merged
merged 38 commits into from
Oct 28, 2024

Conversation

PolarBean
Copy link
Contributor

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.

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 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?

@adamltyson
Copy link
Member

should we explain somewhere how to include additional references or is that considered out-of-scope here?

Should we have an explicit section like:

# **** OPTIONAL *****
additional reference stuff

PolarBean and others added 16 commits September 11, 2024 15:03
@PolarBean
Copy link
Contributor Author

PolarBean commented Sep 11, 2024

should we explain somewhere how to include additional references or is that considered out-of-scope here?

Should we have an explicit section like:

# **** OPTIONAL *****
additional reference stuff

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?

@PolarBean
Copy link
Contributor Author

@alessandrofelder I've addressed your comments and am ready for this to be re-reviewed once you have the time :)

@adamltyson
Copy link
Member

this seems like a good solution but i cant see any instances of multiple an atlas that has multiple citable documents (if this is what you're referring to). How should this be implemented?

Here we mean multiple reference images (i.e. templates, we should rename this!). I think the AZBA has 2 reference images.

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 @PolarBean - looking good!
Two more tiny comments, then we can merge this.

@alessandrofelder
Copy link
Member

I am tempted to merge this at some point this week, and open a new issue for moving it away from AllenSDK - any objections @PolarBean @adamltyson ?

@adamltyson
Copy link
Member

No argument from me.

@PolarBean
Copy link
Contributor Author

Works for me!

@alessandrofelder alessandrofelder merged commit e718f4c into brainglobe:main Oct 28, 2024
11 of 13 checks passed
@PolarBean PolarBean deleted the atlas_packaging_template branch October 28, 2024 20:59
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