jonescompneurolab
[MRG] Add celltypes to `CellResponse` interface and `Network.rename_cell()`
This PR was originally much more complex, but has been greatly simplified by moving the discussion of hardcoded-celltypes to #972. This PR has now been changed to simply be an attempt to merge in the parts of #702 that can be made to pass our tests.
It is known that the tests are failing "across the board" after merging in #702 's prior work. These are mostly tied to new cell_types arguments and argument order for CellResponse and Network. I should be able to fix these issues as of tomorrow.
Edit: The text formerly in this this comment has been moved to https://github.com/jonescompneurolab/hnn-core/issues/972#issuecomment-2605753819 .
I agree, CellResponse's handling of cell_types of very confusing. I like solution (1) where you convert everything to "spike_types" instead, which when compared internally to _cell_type_names can be used to delineate between cell vs. drive spikes.
@asoplata I'll go through this PR ... just wanted to mention that you should avoid making branches on upstream as much as possible. It keeps the upstream repo clean. The only branches you want to be making are for each of the releases. Anyone with admin writes can push to any PR, so you can just start with a normal PR on your fork and anyone with push access to hnn-core can also push to the branch on your fork
I agree cell_types are somewhat of a misnomer. But just be wary of changing things that may already be used in other people's codes. If it's really inconvenient, make sure to deprecate it ... I can help you organize a proper deprecation warning if you have never done it before.
For this PR, I would recommend creating a plan which breaks down the contribution into smaller PRs each of which are easy to review and merge. My initial thinking was to make net._add_cell_type public, then see how much of the pipeline works for a new cell type ... e.g., does the plotting of spikes work etc. Once you have that work, then in a separate PR you can implement more advanced features
@asoplata I'll go through this PR ... just wanted to mention that you should avoid making branches on
upstreamas much as possible. It keeps the upstream repo clean. The only branches you want to be making are for each of the releases. Anyone with admin writes can push to any PR, so you can just start with a normal PR on your fork and anyone with push access to hnn-core can also push to the branch on your fork
@jasmainak Yeah, I'm not sure why I thought this approach was the best. Apologies! Originally, I thought that for a potentially very-large feature with lots of development on it, it would be good to make a "feature branch" out of #702, and then use this PR as the main place for organizing our hardcoding-removal efforts; other, smaller PRs could go into this feature-branch before the whole branch goes to master. The more I thought about it after your comment, however, the more I realized the disadvantages, including:
- devs would have to do things in a nonstandard way (at all)
- devs would have to target their own PR-branches to a different branch that is not necessarily in-sync with master,
master<->feature<->individualPR, - devs would have to build and rebase onto a potentially-volatile feature-branch instead of
master. In contrast, I would argue that helping people rebase their own changes on top ofmasterlocally is, itself, at least medium-complexity. Adding more complexity to that is a bad idea. - Even if is difficult to break down a major feature into smaller parts, doing so makes
master's history significantly easier to comprehend (i.e. the same reason we use "linear history" in the first place), and allows better and independent analysis of each component of said feature (each individual PR). It also means there is just "one workspace context" so to speak. - Intermingling of the actual code for the PR versus discussion (especially about things not located specifically in this PR) would make the conversation harder to read.
- etc.
Instead, I should have fixed and merged #702, then made an Issue unbound to specific PRs. As such, I will move the discussion points over to a new issue #972, and then we'll go about it the usual way: many smaller PRs into master.
I will make one more small commit to make your stylistic change, then this PR should actually be merged, and the branch deleted.
@jasmainak Okay, once this passes the tests, this PR should be ready for merge then branch deletion. Thanks!
Sorry I haven't had the time to look yet! Please feel free to ping me again on the relevant PRs if I don't respond back in a few days.
No rush, @jasmainak ; just so you know, I had to force-push solely due to a conflict in whats_new. Currently, the only differences between the version you reviewed 2 weeks ago are that whats_new change, and the additional test suggestion by Ryan.
@asoplata the way I envision this to work is the following. You can do something like this:
net1 = jones_2009_model()
net2 = net.copy()
net1.rename_cells({f'old_name_col1': old_name for old_name in net1.cell_names})
net2.rename_cells({f'old_name_col2': old_name for old_name in net2.cell_names})
# now add the connections between the two networks -- separate PR
net = net1 + net2
net.add_connection('L5_pyramidal_col1', 'L5_pyramidal_col2', ...)
Somewhat unrelated note: Adding support for multi-network simulation (including distinct long-range connections between networks) is one of our GSoC 2025 project proposals: https://github.com/jonescompneurolab/hnn-core/wiki/GSoC-Ideas-2025
Had to force-push due to rebase conflict with master (the Sphinx theme change). I believe I have addressed the remaining comments in the most recent 2 commits, and barring future comments, this should be ready for merge.
Re: plotting tests, see my response above. I added several more tests that should satisfy overall. I was able to get plot_spikes_raster working with custom cell types, but only when forcing the network to have spikes: it only allows for custom celltype names if there are spikes present by those names in CellResponse (see #972). As an example, here is a live result with new names:
Re: connectivity-clearing, I made major improvements to the newly-renamed Network.rename_cell_type(). It no longer clears the connectivity, but instead it now dives into all common attributes that make use of custom cell_type names that I could find, including connectivity, drives, and biases, and then updates the appropriate cell type names. There may be other uses of cell type names in the ultimate Network data, but I could not find any across multiple different networks with all the variations I could think of. The only exception is the Cell.name children of Network, but these were not updated for two reasons: 1. Cell.name currently has no way to be updated after initialization, and 2. IIRC these are important for actual NEURON cell building, and any renaming needs to be done carefully in league with #972.
Rebased to remove conflict with whats_new.md. @jasmainak Can you please review this? I believe I have addressed all your comments.
apologies @asoplata for the slow review! The code is beginning to take really good shape ...
Okay @jasmainak , I believe I've either incorporated (and resolved) all of your changes, or responded to them.
@jasmainak Have you had a chance to look at the final response comments? I think I addressed your concerns
All concerns have been addressed, including adding __repr__ tests. Thanks @wagdy88 and @jasmainak !