fix inconsistent graph properties between the SG and the MG API #3757
Conversation
…8_fix-inconsistent-graph-properties
| if self.properties.renumbered: | ||
| edgelist_df = self.renumber_map.unrenumber( | ||
| edgelist_df, simpleGraphImpl.srcCol | ||
| ) | ||
| edgelist_df = self.renumber_map.unrenumber( | ||
| edgelist_df, simpleGraphImpl.dstCol | ||
| ) | ||
|
|
||
| # FIXME: revisit this approach | ||
| print("the mapping = ", self.renumber_map.internal_to_external_col_names) |
There was a problem hiding this comment.
can you remove this debug print?
There was a problem hiding this comment.
Sure I was going to remove it
| # FIXME: instead of hardcoded value, it should be 'simpleGraphImpl.srcCol' | ||
| # but there is no way to retrieve it with the current API |
There was a problem hiding this comment.
I think the NumberMap instance is constructed with the src and dst column names (self.src_col_names, self.dst_col_names), can you use those?
There was a problem hiding this comment.
Right I didn't catch that those were also captured in NumberMap. Thanks
There was a problem hiding this comment.
Actually no. Now I remember why I added the FIXME stating that we can't retrieve those column names. self.src_col_names and self.dst_col_names are the column names passed by the user as input and are not the internal referenced ones in simpleGraphImpl.srcCol and simpleGraphImpl.dstCol. With our current API, those ones are not retrievable in NumberMap.
There was a problem hiding this comment.
But I think I can capture that in NumberMap from simpleGraph
|
|
||
| # FIXME: instead of hardcoded value, it should be 'simpleGraphImpl.srcCol' | ||
| # but there is no way to retrieve it with the current API | ||
| if column_name in [self.renumbered_src_col_name, "src"]: |
There was a problem hiding this comment.
I thought we had "src" and "dst" defined somewhere so that we always used a reference rather than a string that we could type wrong
There was a problem hiding this comment.
Yes we had it defined in simpleGraph but I didn't see that it was also stored in NumberMap (That's why I added a fixme so that I can come and revisit it) until @rlratzel pointed that out. It should be fixed in the next commit
There was a problem hiding this comment.
Actually no, those are not reference in NumberMap. Now I remember why I added the FIXME stating that we can't retrieve those column names. self.src_col_names and self.dst_col_names are the column names passed by the user as input and are not the internal referenced ones in simpleGraphImpl.srcCol and simpleGraphImpl.dstCol. With our current API, those ones are not retrievable in NumberMap.
| @@ -118,7 +118,7 @@ def calc_betweenness_centrality( | |||
| ) | |||
|
|
|||
| M = G.to_pandas_edgelist().rename( | |||
| columns={"src": "0", "dst": "1", "weights": "weight"} | |||
| columns={"src": "0", "dst": "1", "wgt": edge_attr} | |||
There was a problem hiding this comment.
this is another place where I would like to see a references. We have switched from "wt" to "weight" to "weights", and now to "wgt". by using a reference we would only change the column names in one place
There was a problem hiding this comment.
When using the datasets API to get a graph with graph_file.get_graph, this implicitly has the column names hardcoded to src, dst, and wgt. For instance the karate datasets in the yaml file
…8_fix-inconsistent-graph-properties
|
added the |
|
/merge |
Several graph methods are failing, some being an effect of migrating away from cython.cu renumbering.
This PR fixes couple graph methods and fixes the inconsistency in results returned by the SG and MG API
closes #3740
closes #3766