-
Notifications
You must be signed in to change notification settings - Fork 12
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
.connect()
and fully_connect()
methods
#197
Conversation
4d2f73e
to
cc7efc9
Compare
c91917d
to
6a74de4
Compare
2b8f177
to
4b3a040
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Minor suggestions for improvement in inline comments.
def connect(self, post: "CompartmentView", synapse_type): | ||
"""Connect two compartments with a chemical synapse.""" | ||
synapse_name = type(synapse_type).__name__ | ||
is_new_type = True if synapse_name not in self.pointer.synapse_names else False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would start with a short paragraph as a comment describing the implementation strategy to scaffold the information for any dev, reviewer, user.
# High-level strategy
# We need to first check if the compartment [branch, cell, network ?] already has a type of this synapse, else we need to register it as a new synapse in a bunch of dictionaries which track synapse parameters, state and meta information [Is this correctly inferred from the code?].
# Next, we register the new connection in the synapse dataframe.
# Then, we update synapse parameter and state arrays with the new connection.
# Finally, we update synapse meta information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, copied it almost verbatim into the docstring. And yes, your description is correctly inferred from the code.
def loc_of_index(global_comp_index, nseg): | ||
"""Return location corresponding to index.""" | ||
index = global_comp_index % nseg | ||
possible_locs = np.linspace(1, 0, nseg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why reversed from 1 to 0 instead of 0 to 1? Cause I would expect that index 0 corresponds to location 0.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, indeed, this is unintitive. See #30
.connect()
method.connect()
and fully_connect()
method
.connect()
and fully_connect()
method.connect()
and fully_connect()
methods
7c08004
to
f1f8303
Compare
Closes #180. FYI @jnsbck @kyralianaka @lappalainenj
Single connections
Following the API of NEURON, this PR implements the following:
which is equivalent to:
Fully (or sparsely) connected layers