xgi-org / xgi

CompleX Group Interactions (XGI) is a Python package for higher-order networks.
https://xgi.readthedocs.io
Other
178 stars 27 forks source link

max_order not taking attributes into account in add_simplices_from #218

Closed maximelucas closed 1 year ago

maximelucas commented 1 year ago

Currently, the input of add_simplices_from needs to be a list of simplices, each simplex of the format (1,2,3) or with attributes, say (1,2,3, {"color": "red}. The len of the simplex is then 3 or 4 even though its size is 3. The initial part of the function, dealing with the max_order parameter, does not take the possibility of the attributes into account:

https://github.com/ComplexGroupInteractions/xgi/blob/254557062b73fb5a93efeb74026534d6347207db/xgi/classes/simplicialcomplex.py#L232-L240

Adding the simplex (1,2,3, {"color": "red"}) to a simplicial complex, we expect it to be added itself with its attribute, and all its edges and singleton nodes without attributes.

The actual result is the following:

from itertools import combinations

edges = [(1,2,3, {"color": "red"})]
max_order = 2

for edge in edges:
    if len(edge) >  max_order + 1:
        print(list(combinations(edge,  max_order + 1)))

## --> [(1, 2, 3), (1, 2, {'color': 'red'}), (1, 3, {'color': 'red'}), (2, 3, {'color': 'red'})]

The simplex (1, 2, 3) will still be added, but without attributes. Its edges will be added, but with the attributes. (EDIT: S.add_simplices_from([(1,2,3, {"color": "red"})], max_order=2) adds the pairwise edges without attributes, not sure why.) Note that we cannot see this error by looking only at S.edges.members() because the members are all correct.

More generally, are we not making our lives difficult by allowing some input edges to have attributes and others not? If that's a common let's keep it. If not, we could have a check at the beginning of the function to check if there are attributes, and deal with it more easily I guess.

maximelucas commented 1 year ago

I guess one easy fix is to start by adding an empty attribute dict {} to all edges that don't have one (which is done after the max_order lines, and was originally the first thing in the function).

leotrs commented 1 year ago

I very very very much dislike the format {1, 2, 3, att_dict}. I vote we scrap it entirely.

Compare SimplicialComplex.add_simplices_from to Hypergraph.add_edges_from. The latter was completely overhauled after the two classes were defined. Hypergraph.add_edges_from does not support the format {1, 2, 3, att_dict} and neither should SimplicialComplex.add_simplices_from.

maximelucas commented 1 year ago

Oh true, we completely forgot to update the add_simplices_from the same way. But okay, great, I prefer the new format too.