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

H01 interface #162

Merged
merged 4 commits into from
Oct 13, 2024
Merged

H01 interface #162

merged 4 commits into from
Oct 13, 2024

Conversation

jinhan
Copy link
Contributor

@jinhan jinhan commented Sep 11, 2024

H01 interface based on the latest master branch

  • add navis/interfaces/h01.py
  • add docs/examples/3_interfaces/plot_04_interface_h01.py

@schlegelp
Copy link
Collaborator

schlegelp commented Sep 12, 2024

Thanks for this PR. I left a few comments above. In particular the docstrings need a bit of work as they still contain references to the MICrONS dataset.

Additionally I have two questions/suggestions:

  1. You seem to be using your own CAVE service (global.brain-wire-test.org) and I was wondering whether users had to keep that in mind if they wanted to store their token manually? I'm guessing it changes the name of the secrets file from global.daf-apis.com-cave-secret.json to global.brain-wire-test.org-cave-secret.json? Perhaps you could add a note to the tutorial. I'd use an admonition - something like this:
    !!! note "Saving your token manually"
        If you want to save your token manually, you have to ...
  2. There is quite a bit of shared functionality now between the H01 and the MICrONS interface. If you have the appetite for it, it would be nice to refactor a little to reduce code duplication. For example, we could minimally put the _chunks_to_nm and _fetch_single_neuron functions into a cave_utils.py module and re-use them in both h01.py and microns.py:
     └── interfaces
         ├── h01.py 
         ├── microns.py
         └── cave_utils.py  <- put shared functions here
    Most of the other functions could also be put into cave_utils and just thinly wrapped in h01.py. For example:
     def fetch_neurons(
         x,
         *,
         lod=2,
         with_synapses=True,
         datastack="h01_c3_flat",
         parallel=True,
         max_threads=4,
         **kwargs,
     ):
         """Fetch neuron meshes.
         ...
         """
         return cave_utils._fetch_neurons(
             x,
             lod=lod,
             with_synapses=with_synapses,
             datastack=datastack,
             parallel=parallel,
             max_threads=max_threads,
             **kwargs,
         )

None of the above are mission critical and I'm happy to take a stab myself later.

As an aside: unfortunately, the tutorial tests are currently only run on push but not on pull_request. That's my fault - I will fix that once this PR is merged.

@jinhan
Copy link
Contributor Author

jinhan commented Sep 17, 2024

Thank you for the comments.

  1. I also found that the H01 interface doesn't need the Auth class. With the H01's server_address, I can create a CAVE client. In addition, I checked that the token is saved in global.brain-wire-test.org-cave-secret.json under the .cloudvolume/secrets folder.

  2. I refactored h01.py and microns.py by introducing cave_utils.py, as you suggested.

@schlegelp
Copy link
Collaborator

Sorry, I haven't had the time to properly look through the PR yet. Will try to do that in the second half of next week!

@jakobtroidl
Copy link

@schlegelp @jinhan, i just wanted to check in on this. @schlegelp did you have a chance to look at the updated code already?

@schlegelp
Copy link
Collaborator

Thanks for the prod. I'm on it now. I'm planning a new release sometime this week.

Copy link
Collaborator

@schlegelp schlegelp left a comment

Choose a reason for hiding this comment

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

This looks good to me. I made a few comments about docstrings that now only exists in cave_utils but not in the actual user-facing functions. Otherwise, I think this is good to go.

docs/examples/3_interfaces/plot_04_interface_h01.py Outdated Show resolved Hide resolved
docs/examples/3_interfaces/plot_04_interface_h01.py Outdated Show resolved Hide resolved
navis/interfaces/h01.py Outdated Show resolved Hide resolved
navis/interfaces/h01.py Outdated Show resolved Hide resolved
docs/examples/4_remote/plot_04_remote_h01.py Outdated Show resolved Hide resolved
navis/interfaces/h01.py Outdated Show resolved Hide resolved
navis/interfaces/h01.py Outdated Show resolved Hide resolved
navis/interfaces/h01.py Show resolved Hide resolved
navis/interfaces/microns.py Show resolved Hide resolved
@jinhan jinhan requested a review from schlegelp October 11, 2024 21:28
Copy link
Collaborator

@schlegelp schlegelp left a comment

Choose a reason for hiding this comment

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

Looks good to me now, thanks!

@schlegelp schlegelp merged commit 5b370c5 into navis-org:master Oct 13, 2024
10 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.

3 participants