-
Notifications
You must be signed in to change notification settings - Fork 11
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
Better viewing #447
Merged
Merged
Better viewing #447
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
9 tasks
jnsbck
force-pushed
the
better_viewing
branch
from
October 15, 2024 14:58
01f6ecd
to
a559caf
Compare
Awesome! Thanks for the review. Changed a bunch of stuff and left some things open for further discussions. |
michaeldeistler
approved these changes
Oct 23, 2024
This was referenced Oct 23, 2024
Closed
This was referenced Oct 25, 2024
jnsbck
added a commit
that referenced
this pull request
Nov 3, 2024
* 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
Merged
michaeldeistler
pushed a commit
that referenced
this pull request
Nov 13, 2024
* 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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I have been working on this for a while, but I think I have figured 99% of the difficult things out, so it's time to make a PR to get your feedback on it and to slowly integrate it into
main
. I suspect it will take a while since it messes with a lot of stuff under the hood, but in the long run it should make life much easier.The aim of this PR is to change how
View
works inJaxley
. At the momentModule
andView
are two different objects and hence they behave very differently. For example aView
cannot be integrated. Under the hood treatingModule
andView
differently also leads to a lot of duplicate methods and boilerplate since all methods that we want to have in bothModule
andView
have to be implemented inModule
and wrapped once to expose them forModule
and once to expose them forView
via the pointer that points back toModule
. I therefore propose to handle viewing differently, namely:View
a sublcass ofModule
. This means ifModule
returns aView
all attrs and methods still work.View
controls how the attrs ofModule
are presented, i.e.cell.branch(0).nodes
will return a dataframe with the subset of nodes that are part of branch 0. Which part of aModule
is in view is kept track of internally by an index.nodes
.Since
View
is aModule
, viewing is recursive, i.e.View(View(Module))
works as well. Furthermore and importantly, the current API stays the same for the user.This is very cool, because it allows for a lot of new features:
Indexing is now more flexible, we could i.e. sample them and index into the Module using
at
or index into comps straight fromNetwork
.Additionally, keeping track of global and local indexes in nodes allows to select which "scope" to use when indexing.
Its now also easy to add context management and iterables.
Groups, channels and synapses now basically behave the same and can be called at any time.
And indexing into edges/synapses is now also possible using
edge
And arguably the coolest feature imo is being able to copy / extract elements of a Module, since Views <=> Module, which could be used to simulate only parts of a network for example.
Very curious to hear your feedback or suggestions you have. If you wanna play around with this yourselves, you can pull from this branch.
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