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.

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

WIP: refactoring hard coded parts in renaming cells in HNN #702

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

wagdy88
Copy link

@wagdy88 wagdy88 commented Dec 25, 2023

Work in Progress to refactor the hard-coded parts in the code for renaming cells in HNN.

@rythorpe
Copy link
Contributor

Glad to see you got everything setup @wagdy88! Next step is to start working through Network class to get it to create an instance with your updated cell name without throwing any errors. For starters, we might want to consider moving _create_cell_coords to within the Network class so that it can access the Network.cell_types automatically.

@rythorpe
Copy link
Contributor

Then, you could refactor the logic for assigning a cell's position within the column according to cell.name. The idea is that the keys of Network.cell_types correspond to cells' network-level names, which are mutable, but that an individual cell's name attribute corresponds to it's cell-level name and should remain immutable because this value determines where it gets placed within the network.

@rythorpe rythorpe mentioned this pull request Dec 27, 2023
hnn_core/network.py Outdated Show resolved Hide resolved
hnn_core/network.py Outdated Show resolved Hide resolved
@wagdy88 wagdy88 force-pushed the refactor_cell_name branch 2 times, most recently from 0f22c81 to 5ca37a5 Compare January 11, 2024 20:24
@@ -387,20 +403,19 @@ def __init__(self, params, add_drives_from_params=False,
# cell counts, real and artificial
self._n_cells = 0 # used in tests and MPIBackend checks
self.pos_dict = dict()
self.cell_types = dict()
# COMMENTED By WAGDY #self.cell_types = dict()
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove line.

Comment on lines 1110 to 1114
#if cell_name == "L2_basket":
# self.gid_ranges[cell_name] = range(0,35)
# elif cell_name == 'L2_pyramidal'
# .....
# if self.gid_ranges[cell_name]
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove unless you have a plan for this later.

Comment on lines 1366 to 1369
Args:
original_name (string): The original cell
name in the network to be changed
new_name (string): The desired new cell name in the network
Copy link
Contributor

Choose a reason for hiding this comment

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

Follow same format as is elsewhere in the repo for readability.

def __init__(self, params, add_drives_from_params=False,
legacy_mode=False):
# Save the parameters used to create the Network
_validate_type(params, dict, 'params')
self._params = params
# Update the cell names -if needed
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove line.

@@ -363,7 +378,8 @@ def __init__(self, params, add_drives_from_params=False,
stacklevel=1)

# Source dict of names, first real ones only!
cell_types = {
# adding self before cell_types to make it an instance attribute
self.cell_types = {
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to consider turning this into an OrderedDict to be explicit about order mattering here. Either that or we need to find a way to make order at this point irrelevant.

Copy link
Contributor

Choose a reason for hiding this comment

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

Resolved in conversation with @wagdy88: our solution will be to maintain the original order of Network.gid_ranges after renaming, but not necessarily Network.cell_types or Network.pos_dict.

hnn_core/viz.py Outdated
Comment on lines 526 to 534
cell_types = cell_response._cell_type_names
cell_type_colors = ['r', 'b', 'g', 'w']
# Ensure cell_types list matches the size of cell_type_colors dictionary
if len(cell_types) != len(cell_type_colors):
raise ValueError("cell_types list must "
"match the number of items in cell_type_colors.")

cell_types = ['L2_basket', 'L2_pyramidal', 'L5_basket', 'L5_pyramidal']
cell_type_colors = {'L5_pyramidal': 'r', 'L5_basket': 'b',
'L2_pyramidal': 'g', 'L2_basket': 'w'}
# Create a new dictionary with the new keys and old values
new_cell_type_colors = dict(zip(cell_types, cell_type_colors))
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should use a default colormap here and ask users to enter a dict of colors if they want a specific cell_type -> color mapping. @ntolley wdyt?

hnn_core/viz.py Outdated
Comment on lines 590 to 594
# re-arranged cell_colors to match old colors dictionary
cell_colors = ['m', 'c', 'r', 'b']
colors = dict(zip(net.cell_types.keys(), cell_colors))
cell_markers = ['x', '^', 'x', '^']
markers = dict(zip(net.cell_types.keys(), cell_markers))
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here as in plot_spikes_raster.

Comment on lines +82 to +88
# WAGDY: could try to create a dictionary
# of src_cell and target_cell and then a
# function that takes default cell names
# from this dictionary and overrides it
# with user cell names
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove

wagdy88 and others added 5 commits August 11, 2024 12:54
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
@wagdy88 wagdy88 force-pushed the refactor_cell_name branch from 9e4ad07 to 6b1d018 Compare August 11, 2024 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants