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

Adding weightHP query option to functions #29

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Adding weightHP query option to functions #29

wants to merge 2 commits into from

Conversation

umayaml
Copy link

@umayaml umayaml commented Oct 20, 2021

Adding weightHP query option to fetch_shortest_paths and fetch_simple_connections, etc.

See #25

TODO

The following functions will all need to be updated to support a high-precision option.

  • fetch_adjacencies()
  • fetch_traced_adjacencies()
  • fetch_simple_connections()
  • fetch_common_connectivity()
    • Add roi parameter, too.
    • Fix pre-existing bug (see below).
  • fetch_shortest_paths()
  • fetch_output_completeness()
  • fetch_downstream_orphan_tasks()
    • Let's not bother with this one for now.

Updating the following functions will require a more significant change to the API, since they don't use weightHP. They inspect synapses individually, but I think it would be best to aim for conceptual consistency with the above functions if possible. Perhaps these updates should be deferred for now, and implemented in a separate pull request.

  • fetch_synapse_connections()
  • fetch_connection_mitochondria()

Documentation updates:

  • Update docstrings for all edited functions.
  • Update docs/source/changelog.rst
  • Maybe add a new entry in docs/source/faq.rst?

Lowell Umayam added 2 commits October 6, 2021 21:36
…paths. If set to true, will use weightHP value in the Neuron to Neuron relationship instead of weight value.
@stuarteberg
Copy link
Collaborator

Thinking about this a little more...

Right now, the neuprint datamodel only provides weight and weightHP, but soon we'll be introducing weightHR. To future-proof this API for those changes, perhaps we should rename the argument to weight_tuning (or a better name if you can think of one). Instead of True/False, it would accept a string, which must be either default, high-precision, or (eventually) high-recall.

Also, as a side note, I'm wondering if we should make this new argument a keyword-only argument, just to avoid breaking existing code that calls it using the current argument order.

On a related point, apparently fetch_common_connectivity() contains a pre-existing bug, because it calls fetch_simple_connections() with the wrong arguments. Can you fix that while you're at it?

And on a related point to that: I think fetch_common_connectivity() should probably be updated to accept the new weight_tuning argument, and (now that I think about it) also an roi argument.

@stuarteberg
Copy link
Collaborator

I've edited the PR description above to add a TODO checklist.

Thanks again for posting this draft PR immediately for the first two functions (as we discussed), rather than implementing everything in one go. This gives us a chance to nail down some API details before you change all of the relevant functions.

@stuarteberg
Copy link
Collaborator

stuarteberg commented Nov 30, 2021

@umayaml I reorganized the contents of queries.py, splitting it across multiple files. When you come back to this PR, you should rebase from the current master branch (or start over).

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