refactor(summarize_networks): use col names directly #385
Conversation
Also pins median degree to be an integer.
tristan-f-r
added
the
refactor
Documentation build overview
Show files changed (2 files in total): 📝 2 modified | ➕ 0 added | ➖ 0 deleted
|
| nw_info.append(current_network_info) | ||
|
|
||
| # Prepare column names | ||
| col_names = ['Name', 'Number of nodes', 'Number of edges', 'Number of connected components', 'Density', 'Max degree', 'Median degree', 'Max diameter', 'Average path length'] |
There was a problem hiding this comment.
Do we need col_names? Could we get that directly from the keys now?
There was a problem hiding this comment.
We could. Should we? It's nice to have an explicit list of all of the columns - the column duplication may help here.
There was a problem hiding this comment.
I was under the assumption that part of this PR was to get rid of the col_names part, otherwise I don't fully understand why this is being changed to a dict.
There was a problem hiding this comment.
Oh - it's just to remove the cur_nw_info variable indirection in favor of directly referencing keys.
There was a problem hiding this comment.
I still don't get the point of this PR. Why do we not want to use cur_nw_info? You mentioned it was bad in the other PR, but there was no reasoning as to why it's bad.
There was a problem hiding this comment.
I'll close this PR since I don't want to push this change through if it isn't straightforward, since it's a small refactor after all 👍
My reasoning for not using cur_nw_info is that it's hard to follow the graph statistics when they are named by variables instead of their string header names,
| for node_list in nodes_by_col: | ||
| num_nodes = len(set(nw).intersection(node_list)) | ||
| cur_nw_info.append(num_nodes) | ||
| current_network_info.append(num_nodes) |
There was a problem hiding this comment.
Is there a way to make this a key value pair too using nodes_by_col_labs?
| # Algorithm parameters have format { algo : { hashcode : { parameter combos } } } | ||
| param_combo = algo_params[algo][hashcode] | ||
| cur_nw_info.append(param_combo) | ||
| current_network_info.append(param_combo) |
There was a problem hiding this comment.
Same with param_combo? (making it a key value pair)
tristan-f-r
added
the
awaiting-author
tristan-f-r
removed
the
awaiting-author
3 participants
This refactor prefers duplicating column names over using shortened variables, helping to make the distinction between local variables and variables exported to the summary dataframe.
(This was from #368 (comment))