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

Skeleton service incorporation into CAVEclient is working, at least in certain respects #212

Merged
merged 45 commits into from
Sep 9, 2024

Conversation

kebwi
Copy link
Collaborator

@kebwi kebwi commented Aug 20, 2024

I'd like to merge these CAVEclient changes in but would definitely appreciate others looking it all over to make sure it doesn't look like it would break anything important.

@ceesem
Copy link
Collaborator

ceesem commented Aug 20, 2024

A few comments:

  1. We should have basic tests to be included before it's merged to avoid simple breaking errors.
  2. While it's not consistent with the older code, it would be nice if new code were type hinted as we work on an overhaul to "modern" stylings (same with the version locking system)

@ceesem
Copy link
Collaborator

ceesem commented Aug 20, 2024

Looking at the endpoints that are available, I wonder if it wouldn't make sense to add something like:

  1. skeleton_exists(self, root_ids, version=None, ...) : This would take a list of root ids and tell you which all have existing skeletons, which could be useful for organizing bulk analysis and debugging.
  2. skeletonization_versions(self, datastack_name=None) : This would return a list of keys for the versions that are available and descriptions of each version, including maybe links to the exact code.

I also wonder if there shouldn't be a multithreaded bulk download function.

@ceesem
Copy link
Collaborator

ceesem commented Aug 20, 2024

Also note that there are currently are some linting errors.

@kebwi
Copy link
Collaborator Author

kebwi commented Aug 21, 2024

Looking at the endpoints that are available, I wonder if it wouldn't make sense to add something like:

  1. skeleton_exists(self, root_ids, version=None, ...) : This would take a list of root ids and tell you which all have existing skeletons, which could be useful for organizing bulk analysis and debugging.
  2. skeletonization_versions(self, datastack_name=None) : This would return a list of keys for the versions that are available and descriptions of each version, including maybe links to the exact code.

I also wonder if there shouldn't be a multithreaded bulk download function.

Saving for another day. I'd like to wrap up this PR for now.

@kebwi
Copy link
Collaborator Author

kebwi commented Aug 22, 2024

I object to the lint tests requiring ruff formatting that butchers the code and requires us to work with a visual morass instead of formatting code in a way that is easier to visually comprehend and understand.

@fcollman fcollman merged commit 97be046 into master Sep 9, 2024
16 checks passed
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.

6 participants