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 support for custom offline atlases #497

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

Conversation

carlocastoldi
Copy link
Contributor

With this small PR, i want to enable atlas developers to test their atlases using BrainGlobe's pipeline.
As of now, if the atlas is not official (i.e. present on the Gin repository), it updates the last_versions.conf file overriding any local changes that may have added a new custom local atlas.

This PR intends to read the atlases from both the cached file of last_versions.conf and custom.conf file.
custom.conf follows the same structure as last_versions.conf.

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

References

discussed over on Zulip.

How has this PR been tested?

python -c "import brainglobe_atlasapi as bg; print(bg.list_atlases.get_all_atlases_lastversions())"

with custom.conf file both existing and not.

Is this a breaking change?

no

Does this PR require an update to the documentation?

Perhaps?

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

it now reads atlases from cached last_versions.conf and custom.conf files
@carlocastoldi
Copy link
Contributor Author

carlocastoldi commented Feb 10, 2025

i don't think i introduced a bug that makes the test not pass, am i?

It's on atlasgen

@IgorTatarnikov IgorTatarnikov self-requested a review February 11, 2025 11:20
Copy link
Member

@IgorTatarnikov IgorTatarnikov left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for adding this. Just a few semantic comments.

This change should be documented on the website but this can come in a subsequent PR, I'll make an issue to track this.

@@ -59,18 +59,22 @@ def get_local_atlas_version(atlas_name):
def get_all_atlases_lastversions():
"""Read from URL or local cache all available last versions"""
cache_path = config.get_brainglobe_dir() / "last_versions.conf"
custom_path = config.get_brainglobe_dir() / "custom.conf"
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
custom_path = config.get_brainglobe_dir() / "custom.conf"
custom_path = config.get_brainglobe_dir() / "custom_atlas.conf"

Copy link
Contributor Author

@carlocastoldi carlocastoldi Feb 13, 2025

Choose a reason for hiding this comment

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

wouldn't custom_atlases.conf be a better name, then?

Copy link
Member

Choose a reason for hiding this comment

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

custom_atlases.conf also works!

brainglobe_atlasapi/list_atlases.py Outdated Show resolved Hide resolved
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