[MRG] Add celltypes to CellResponse interface and Network.rename_cell()
#970
Conversation
|
It is known that the tests are failing "across the board" after merging in #702 's prior work. These are mostly tied to new |
|
Edit: The text formerly in this this comment has been moved to #972 (comment) . |
|
I agree, |
|
@asoplata I'll go through this PR ... just wanted to mention that you should avoid making branches on |
|
I agree 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 |
hnn_core/network.py
Outdated
| len(self.pos_dict['L5_basket']))) | ||
| return '<%s | %s>' % (class_name, s) | ||
| # Dynamically create the description based on the current cell types | ||
| descriptions = [] |
There was a problem hiding this comment.
| descriptions = [] | |
| descriptions = list() |
just a stylistic suggestion, it's hard to distinguish [] and {} but list() and dict() are fairly clear and unmistakable.
This includes many small changes that either fix test-breaking issues for this branch, retain consistency, or revert certain changes in order for us to work on more comprehensive solutions to these hardcoding issues. For `Network._create_cell_coords()`, I reverted the changes, because how we will handle custom celltype positions needs to be considered very carefully and comprehensively on its own. For the first version, we may want to force a user of custom-celltypes to provide their own position data for all their celltypes, in other words passing all the data we currently assemble using https://github.com/jonescompneurolab/hnn-core/blob/master/hnn_core/network.py#L432-L435 . If we did this, we could expose `Network.set_cell_positions()` as a public function to help users make their own positioning, but they would not be required to use that method. This would be significantly less brittle than exposing and maintaining `_create_cell_coords` as an API. For the `cell_types` -> `self.cell_types` changes, I reverted them because `cell_types` is already converted into `self.cell_types` in an organized way in `Network._add_cell_type()`. In `Network.rename_cell()`, simplified type validation checks. In `test_cell_response.py`, cleaned up a little bit and removed "print" debugging from tests. Fixed type tests in `test_network.py` to work with the more standard type validation. In `viz.py::plot_spikes_raster()`, removed the additional color selecting code due to color selection already being present in the function. In `viz.py` in general, removed non-hardcoded mapping between cell type names and colors, due to inconsistency of how colors are set in different plotting functions. Currently, it appears to me that different plotting functions use different colors for the same cell type. For the discussion of `cell_response._cell_type_names` vs `cell_response.cell_types`, see #970 (comment) .
@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
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 I will make one more small commit to make your stylistic change, then this PR should actually be merged, and the branch deleted. |
asoplata
changed the title
CellResponse interface and Network.rename_cell()
|
@jasmainak Okay, once this passes the tests, this PR should be ready for merge then branch deletion. Thanks! |
| for new_name in new_names: | ||
| assert new_name in net.cell_types.keys() | ||
| assert new_name in net.pos_dict.keys() |
There was a problem hiding this comment.
We should also test that original_name isn't a key anymore
There was a problem hiding this comment.
Done in latest commit.
This is a "toy" commit solely for the purpose of creating a new upstream branch.
removing hard coding in _create_cell_coords function and linking this with __init__ in class Network fixing the _create_cell_coords refactoring _create_cell_coords refactoring _create_cell_coords contd removing cell_name from 466 argument correcting styling error and fixing self.post_dict in class network making different corrections in network fixed line 463 working with line 465 network.py still working with line 407 adding a space added self before cell_types commented line 404 and added self to cell_types to make it an attribute to instance network de-indenting line 115 for pos_dict to get input from the whole code de-indenting lines 103 thru 115 in network corrected names of cells to match cell.name added a comment on the plan lines 81 thru 86 in network_models changing cell name to L2_basket added a rename_cell function and _rename_basket subfunction WIP making some re-organizing of comments to rename_cell function rename is working with gid_ranges ordered, still working on visualization fixing a typo modified rename function to order cell_types items visualization raster working and colors matching fixing extra white spaces etc... . Updating plot cell function WIP maintain original order in net.gid_ranges only modify net.gid_ranges in loop to simplify rename_cell cleaning up code modifying the discription removing extra lines fixed color_map in plot_raster marker set to o for all cells adding tests aethetic modification correct elif or error correcting a typo correcting pytests updating pytest fixing network tests cell-repsonse test _add_cell-type
This includes many small changes that either fix test-breaking issues for this branch, retain consistency, or revert certain changes in order for us to work on more comprehensive solutions to these hardcoding issues. For `Network._create_cell_coords()`, I reverted the changes, because how we will handle custom celltype positions needs to be considered very carefully and comprehensively on its own. For the first version, we may want to force a user of custom-celltypes to provide their own position data for all their celltypes, in other words passing all the data we currently assemble using https://github.com/jonescompneurolab/hnn-core/blob/master/hnn_core/network.py#L432-L435 . If we did this, we could expose `Network.set_cell_positions()` as a public function to help users make their own positioning, but they would not be required to use that method. This would be significantly less brittle than exposing and maintaining `_create_cell_coords` as an API. For the `cell_types` -> `self.cell_types` changes, I reverted them because `cell_types` is already converted into `self.cell_types` in an organized way in `Network._add_cell_type()`. In `Network.rename_cell()`, simplified type validation checks. In `test_cell_response.py`, cleaned up a little bit and removed "print" debugging from tests. Fixed type tests in `test_network.py` to work with the more standard type validation. In `viz.py::plot_spikes_raster()`, removed the additional color selecting code due to color selection already being present in the function. In `viz.py` in general, removed non-hardcoded mapping between cell type names and colors, due to inconsistency of how colors are set in different plotting functions. Currently, it appears to me that different plotting functions use different colors for the same cell type. For the discussion of `cell_response._cell_type_names` vs `cell_response.cell_types`, see #970 (comment) .
asoplata
force-pushed
the
refactor-hardcoded-celltypes
branch
from
7028981 to
a55779c
Compare
|
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 |
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.