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.

Sign up for GitHub

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

Jump to bottom

Continuing better Viewing (#447) #453

Merged
merged 17 commits into from
Nov 3, 2024
Merged

Continuing better Viewing (#447) #453

merged 17 commits into from
Nov 3, 2024

Conversation

jnsbck
Copy link
Contributor

@jnsbck jnsbck commented Oct 23, 2024 •
edited
Loading

The following is a copy of the remaining todos from #447

Merging this will make #329, #330, #351 obsolete and close #377, #354, probably #338 and I guess also #133
Will also make #264 and #134 trivial
Also might wanna revisit #291 after this

Comments:

  • Moved _append_multiple_synapses and associated functionality to Network.
  • SynapseView replaced by module.TestSynapse.edge([0,1])

Open Questions:

  • How should we name self.view, self.base, self.at?
  • How should we handle local indices? Should they be recomputed dynamically? I.e.self.comp([2,3,4]).comp(1) or to allow groups to have a local index self.group_name.comp(0)?
  • change name of _cells_in_view to _cell_indices etc.
  • Treat synapse trainables, i.e. edges the same as nodes? which would also allow param sharing
  • Discuss what we want to allow with new View that we didnt before -> diff of NotImplementedError and other things that are now trivial
  • How to treat local inds? Should we just have single local inds col? This would mean doing net.cell([1,2]).comp(0) would only show the first comp of cell 1 rather than the first comp in every branch of cell 1 and 2
  • should controlled_by_param be 0 or np.arange(len(self.nodes)) by default?

Immediate Todos:

Smaller Todos

@jnsbck jnsbck changed the title Continuing #447 Continuing better Viewing (#447) Oct 23, 2024
This was referenced Oct 23, 2024
Closed
Closed
@jnsbck
Copy link
Contributor Author

jnsbck commented Oct 24, 2024

Added a bunch of tests, fixed a few bugs that popped up and generally cleaned up the code. Also integrated a few changes we discussed, like removing inds from the edges dataframe. edge is kept for now. Removing this should be part of #463

@michaeldeistler michaeldeistler mentioned this pull request Oct 25, 2024
Closed
michaeldeistler
michaeldeistler reviewed Oct 25, 2024
View reviewed changes
Copy link
Contributor

@michaeldeistler michaeldeistler left a comment

Choose a reason for hiding this comment

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

Awesome! I did not review in a lot of detail, but my overall feedback would be to (1) write slightly more comprehensive docstrings (outlining also why particular functions are needed) and (2) adding more detail to comments, as many are not immediately clear to me. Let's also add tests for removing recordings, stimuli, and trainables from views. Thanks!

jaxley/modules/base.py Outdated Show resolved Hide resolved
jaxley/modules/base.py Outdated
@@ -179,7 +183,7 @@ def _childviews(self) -> List[str]:
def __getitem__(self, index):
supported_lvls = ["network", "cell", "branch"] # cannot index into comp

# TODO: SHOULD WE ALLOW GROUPVIEW TO BE INDEXED?
# TODO FROM #447: SHOULD WE ALLOW GROUPVIEW TO BE INDEXED?
Copy link
Contributor

Choose a reason for hiding this comment

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

i still do not understand what you mean by that.

Copy link
Contributor Author

@jnsbck jnsbck Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The problem is that for Modules, i.e. net[0,0,0] it is straightforward to lazy index into by just calling net.cell(0).branch(0).comp(0) internally, likewise for net.cell(0)[0,0]. However, for net.cell0 this is not the case, since we cannot tell easily what the user is indexing into. I.e. they might do net.cell0 or net.comp0, which are totally different, but equivalent on the group level. Now should we allow them to still be able to do net.comp0[0,0,0]? Additionally, should we allow net.cell(0).comp0?

Decided to disable it for now and only allow indexing into Modules and Views thereof. I think that is sensible. The user can of course still index into groups however they like, they just have to do it via comp, cell, branch instead of via __getitem__. Lemme know if you disagree with this decision.

jaxley/modules/base.py Outdated Show resolved Hide resolved
jaxley/modules/base.py Show resolved Hide resolved
jaxley/modules/base.py Show resolved Hide resolved
jaxley/modules/base.py Outdated Show resolved Hide resolved
jaxley/modules/base.py Outdated Show resolved Hide resolved
jaxley/modules/base.py Show resolved Hide resolved
tests/test_viewing.py Show resolved Hide resolved
@jnsbck jnsbck mentioned this pull request Oct 28, 2024
Merged
@jnsbck jnsbck force-pushed the continue_better_viewing branch from d922f3d to 559be17 Compare October 29, 2024 09:53
@jnsbck jnsbck force-pushed the continue_better_viewing branch from 559be17 to 2f2ccd1 Compare October 29, 2024 10:18
@jnsbck
Copy link
Contributor Author

jnsbck commented Oct 29, 2024

I have addressed all comments and added a few more things, see comments above.
Also lemme know what you think about #453 (comment).

Thanks for your feedback.

michaeldeistler
michaeldeistler approved these changes Oct 29, 2024
View reviewed changes
Copy link
Contributor

@michaeldeistler michaeldeistler left a comment

Choose a reason for hiding this comment

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

Amazing! Good to go (one minor comment below)!

jaxley/modules/base.py
@@ -41,6 +42,23 @@
from jaxley.utils.swc import build_radiuses_from_xyzr


def only_allow_module(func):
"""Decorator to only allow the function to be called on Module instances.
Copy link
Contributor

Choose a reason for hiding this comment

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

hm maybe this should live in the utils folder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also thought this, but wanted to avoid circular import issues, since isinstance(self, View) is used, which means we would have to import View in utils. Could work around this by using self.__class__.__name__ == "View" but I dislike this a bit. Lemme know your thoughts @michaeldeistler

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, ok...let us keep it where it is then (but maybe move to the bottom of the file?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also does not work, since it's used in Module and needs to be defined above :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One option would be to fold it into Module. Will merge it like it is, feel free to change this if you dislike it.

@jnsbck jnsbck merged commit f04b747 into main Nov 3, 2024
1 check passed
@jnsbck jnsbck deleted the continue_better_viewing branch November 3, 2024 17:10
michaeldeistler pushed a commit that referenced this pull request Nov 13, 2024
@jnsbck @michaeldeistler
Continuing better Viewing (#447) (#453)
aa8d508 
* add: add first version of new tests

* add: add more tests for new view

* fix: add cumsum_nseg to View and remove cell_list in Network after used to init SolveIndexer

* fix: ammend prev commit

* fix: fold todos into funcs

* wip: save wip, rm pre/post,make delete methods local, fixes

* fix: fix trainables and local inds in edges

* fix: fix jit simulate with data_stimulate issues and streamline edges

* fix: make remaining tests pass

* fix: ammend last

* doc: prepare for review

* wip: save wip

* fix: all tests passing, address review comments

* fix/rm: rm test for laxy indexing into groups, and rebase onto main.

* fix: small refactor

* fix: fix move_to issues. caused by not allowing cell.cells

* doc: add comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaeldeistler michaeldeistler michaeldeistler approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow cell.comp(0) or net.branch(0) or net.comp(0)
2 participants
@jnsbck @michaeldeistler