-
Notifications
You must be signed in to change notification settings - Fork 15
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
Network plot #104
Network plot #104
Conversation
Conflicts: bluecellulab/cell/core.py tests/test_cell/test_core.py
050f957
to
0b29cad
Compare
a9645fa
to
98f0b42
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also add unit tests for the 2 functions in graph.py?
I will fix the gitlab ci error.
setup.py
Outdated
@@ -43,7 +43,8 @@ | |||
"pandas>=1.0.0,<3.0.0", | |||
"bluepysnap>=1.0.5,<2.0.0", | |||
"pydantic>=1.10.2,<2.0.0", | |||
"typing-extensions>=4.8.0" | |||
"typing-extensions>=4.8.0", | |||
"networkx" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add the upperbound for networkx as well?
networkx>=3.2.1
or networkx>=3.2.0
?
@ilkilic also having some text on top of the cell containing the graph would be good for the tutorial. |
Conflicts: examples/2-sonata-network/sonata-network.ipynb setup.py
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #104 +/- ##
==========================================
+ Coverage 84.90% 85.14% +0.24%
==========================================
Files 76 78 +2
Lines 5260 5345 +85
==========================================
+ Hits 4466 4551 +85
Misses 794 794 ☔ View full report in Codecov by Sentry. |
Oh I cannot approve since I created the PR :D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me :)
populations = list(set([cell_id.population_name for cell_id in G.nodes()])) | ||
|
||
# Create a color map for each population | ||
color_map = plt.cm.tab20(np.linspace(0, 1, len(populations))) # type: ignore[attr-defined] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to be sure, do we expect populations to be <= 20 in size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, definitely. The populations here are referring to multiple circuits which is usually <=3.
population_color = dict(zip(populations, color_map)) | ||
|
||
# Create node colors based on their population | ||
node_colors = [population_color[node.population_name] for node in G.nodes()] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, are we getting the population name property of each node, or are we getting the population of the cell_id in each node? I am asking because at line 42, we are doing the same thing, but the variable is named cell_id.
It would be good to be precise about what we are doing, for readability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually line 42 is constructing a set
and removing the duplicates. This line 49 constructs a list with duplicates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I know. But what I meant, is that even though in line 42 it is named cell_id
, and here it is named node
, it looks like it is the same variable we access. In G
, we have something like this for each node (cell_id, id, population_name)
, and my question is, when we are accessing population_name
, are we getting the population_name in the node (the 3rd element in my set), or are we accessing cell_id.population_name
(via the 1st element in my set).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh thanks, a good catch. The "label" and "population" attributes of the node are never used in G. There is duplicated information. Instead of this:
G.add_node(cell_id, label=str(cell_id.id), population=cell_id.population_name)
we can just write this:
G.add_node(cell_id)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always the 1st element in your set is used, the 3rd is never used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks for clarifying
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for noticing, let me write a patch
|
||
# Create a legend | ||
for population, color in population_color.items(): | ||
plt.plot([0], [0], color=color, label=population) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case you don't want to create fake plots for the legend: https://matplotlib.org/stable/users/explain/axes/legend_guide.html#creating-artists-specifically-for-adding-to-the-legend-aka-proxy-artists
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
# Add nodes (cells) to the graph | ||
for cell_id, cell in cells.items(): | ||
G.add_node(cell_id, label=str(cell_id.id), population=cell_id.population_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell, when fetching the population_name from G afterwards, population_name is not used, but cell_id.population_name.
Is it useful to add a population_name to G if you only fetch it from G.cell_id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While plotting the plot in the later step the population name gives the color of the node and it is read from G.
No description provided.