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

Question: MultiGraph with to_dict_of_dicts with edge data #4305

Closed
rossbar opened this issue Oct 30, 2020 · 3 comments · Fixed by #4321
Closed

Question: MultiGraph with to_dict_of_dicts with edge data #4305

rossbar opened this issue Oct 30, 2020 · 3 comments · Fixed by #4321
Labels
Question A question about NetworkX or network science in general type: Documentation

Comments

@rossbar
Copy link
Contributor

rossbar commented Oct 30, 2020

I was reviewing the tests for nx.convert.to_dict_of_dicts which led to a question about what the expected behavior of nx.to_dict_of_dicts(G) is when G is a MultiGraph and edge_data is not None.

Things seem to make sense when edge_data is None:

>>> G = nx.MultiGraph()
>>> G.add_edge(0, 1, key='a')
'a'
>>> G.add_edge(0, 1, key='b')
'b'
>>> nx.to_dict_of_dicts(G, edge_data=None)
{0: {1: {'a': {}, 'b': {}}}, 1: {0: {'a': {}, 'b': {}}}}

In the above example, if I call nx.to_dict_of_dicts(G, edge_data={'weight': 1}), my naive expectation was that i would get

{0: {1: {'a': {'weight': 1}, 'b': {'weight': 1}}}, 1: {0: {'a': {'weight': 1}, 'b': {'weight': 1}}}}

Instead, you get:

>>> nx.to_dict_of_dicts(G, edge_data={'weight': 1})
{0: {1: {'weight': 1}}, 1: {0: {'weight': 1}}}

which seems to clobber the multiple edges. The code doesn't have any special cases for MultiGraphs, and I couldn't tell from there and/or docstring whether this behavior is expected.

I expect I'm missing something, so I thought I'd ask! Adding examples to the docstring would also be a nice improvement to show the expected behavior.

@rossbar rossbar added type: Documentation Question A question about NetworkX or network science in general labels Oct 30, 2020
@berlincho
Copy link
Contributor

Hi @rossbar. The original code in convert.py directly replace edge_data with original edge attributes. When the input is multiGraph, it is natural that we lose the multi-edge information.

# original version
# edge_data is not None
for u, nbrdict in G.adjacency():
    dod[u] = dod.fromkeys(nbrdict, edge_data)

I think the naive method to solve this issue is to consider Single-Graph and Multi-Graphs separately. We should keep different edge keys in MultiGraphs, like the modifications below. Hope to get any feedback or good ideas. Thank you.

# revised version draft
# edge_data is not None
for u, nbrdict in G.adjacency():
    if 'Multi' in nx.info(G).split("\n", 2)[1]: # multi-graphs
        udict = {}
        for nbr, edgedict in nbrdict.items():
            udict[nbr] = dict.fromkeys(edgedict, edge_data)
        dod[u] = udict
    else: # single graph
        dod[u] = dod.fromkeys(nbrdict, edge_data)

@dschult
Copy link
Member

dschult commented Oct 31, 2020

First of all, the docs are incorrect when they say that edge_data should be: list, optional. It can actually be anything. It is the value in the dict-of-dict. The text talks about making it the value 1, so it clearly is not intended to be a list. When edge_data is None we put the edge data dictionary into the dict_of_dict structure. That makes this a dict-of-dict-of-dict structure. But the edge_data input allows any single value in the dict-of-dict output. I am NOT saying this is what it should be. Only what it is. Trying to include a dict or list there is misleading because it will be the SAME DICT FOR EVERY EDGE! Changing dod[u][v]['weight'] will change the weight for ALL EDGES simultaneously because there is only one dict.

I suspect historically this was created for symmetry with from_dict_of_dict. And since it wasn't clear what people might want -- except to make a value of 1, the interface left a way to do that (and maybe more). But I suspect that to_dict_of_dict has not been used very often. @berlincho have you used this function?

This is an artifact that shows our initial neglect of multigraphs and edge attributes. I think v3.0 and a reconsideration of the matrix interface might give us a chance to enrich the treatment of attributes. For example, requiring edge_data to be fa function of u,v here could be helpful. But would make the code more tricky... Perhaps the interface from #4217 could be used for this to_dict_of_dict too.

We could also remove the function -- or at least the edge_data optional argument. We've never received any comlaints about it despite it being incorrectly documented and not useful the way it is documented (you'd have the same list for every edge in the dod).

@dschult
Copy link
Member

dschult commented Oct 31, 2020

After more thought: to_dict_of_dict should return a simple dict-of-dict representation of the graph/multigraph.
edge_data should be a singleton (usually True or 1) to indicate an edge. The default can remain that we
create a dict-of-dict-of-dict with edge data in the inner dict. But the docs should change.

Let's change the doc_string of to_dict_of_dict from:

  edge_data : list, optional
       If provided,  the value of the dictionary will be
       set to edge_data for all edges.  This is useful to make
       an adjacency matrix type representation with 1 as the edge data.
       If edgedata is None, the edgedata in G is used to fill the values.
       If G is a multigraph, the edgedata is a dict for each pair (u,v).

to a more accurate and help version including a one-liner to build your own:

  edge_data : singleton, optional
      If provided, the value of the dictionary will be set to `edge_data` for all edges.
      Usual values could be `1` or `True`.
      If edgedata is None, the edgedata dictionary in G is used creating a dict-of-dict-of-dicts.
      Note that if G is a multigraph, this will be a dict-of-dict-of-dict-of-dicts.
      If you want something more custom made try:
      dod = {n: {nbr: custom(n,nbr,dd) for nbr, dd in nbrdict.items()} for n, nbrdict in G.adj.items()}

Maybe the one-liner should be used in an example in the doc_string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Question A question about NetworkX or network science in general type: Documentation
Development

Successfully merging a pull request may close this issue.

3 participants