cytoscape / RCy3

New version of RCy3, redesigned and collaboratively maintained by Cytoscape developer community
MIT License
48 stars 20 forks source link

Sync node/edge_name_to_suid utility functions to include new unique_list=False parameter #139

Closed bdemchak closed 1 year ago

bdemchak commented 3 years ago
If a name occurs multiple times in the list and unique_list=True, a different SUID is returned
for each name instance, provided that the network has enough same-named edges. If not, an error is returned.

If a name occures multiple times but unique_list=False, a list of SUIDs is returned for each name instance. If there
is only one name, the SUID is returned as a scalar.

This is important in create_network_from_data_frames() and get_first_neighbors()

yihangx commented 2 years ago

Fixed.

AlexanderPico commented 1 year ago

This fix (https://github.com/cytoscape/RCy3/commit/0d3af3082b0d34bac0ce6864b60df2fe5fec0ffe) does not produce the desired output and it reverts the fix for #115 (https://github.com/cytoscape/RCy3/commit/7bdd634bfef6051056cd22d5643e717ca2ff5c4c), which attempted to fix #41.

We need to try again to cover all these cases and not leave prior test cases behind.

AlexanderPico commented 1 year ago

@yihangx Let's start with this code again and try to address Barry's request for a new param without breaking other use cases:

.edgeNameToEdgeSUID<-function(edge.names, network=NULL, base.url=.defaultBaseUrl) {
        dict <- getTableColumns('edge',c('SUID','name'),'default',network, base.url)
        test <- vapply(edge.names, function(x){x %in% dict[,'SUID']}, logical(1))
        if(all(test))  #provided SUIDs already!
            return(edge.names)
        sorted.dict <- NULL
        if(length(edge.names) == length(unique(edge.names))){ #unique edge names
            sorted.dict <- dict[match(edge.names, dict$name), ] 
        } else { #multigraph: multiple edges with the same name
            message("Finding unique SUIDs for edges with the same name...\n")
            match_list <- list()
            for(i in seq_along(edge.names)){ #perform match with removal
                name_match <- dict[match(edge.names[[i]], dict$name),]
                match_list[[i]] <- name_match
                dict <- subset(dict, SUID != name_match$SUID)
            }
            sorted.dict <- do.call(rbind, match_list)
        }
        edge.SUIDs <- sorted.dict$SUID
        return(edge.SUIDs)
}
AlexanderPico commented 1 year ago

When we settle on a fix, then let's apply it to .nodeNameToNodeSUID as well.

bdemchak commented 1 year ago

Can you supply a list of cases that should be satisfied once this fix is in and works? I'm not sure how to test py4cytoscape to make sure it's working.

AlexanderPico commented 1 year ago

In addition to the cases you provided when opening this ticket, the only other case I have is from #41

graph <- graph_from_data_frame(data.frame(v1 = rep("A", 2),
                                          v1 = rep("B", 2),
                                          type = c("E1", "E2")))
createNetworkFromIgraph(graph, "graph")

Should end up with two edges with the same name but with different type values: E1 and E2

AlexanderPico commented 1 year ago

@yihangx This fix (https://github.com/cytoscape/RCy3/commit/0d3af3082b0d34bac0ce6864b60df2fe5fec0ffe) does not produce the desired output and it reverts the fix for https://github.com/cytoscape/RCy3/issues/115 (https://github.com/cytoscape/RCy3/commit/7bdd634bfef6051056cd22d5643e717ca2ff5c4c), which attempted to fix https://github.com/cytoscape/RCy3/issues/41.

We need to try again to cover all these cases and not leave prior test cases behind.

yihangx commented 1 year ago

@AlexanderPico Sure. I am working on this one.