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

Add ability to generate graphs based on correlations of sequences #25933

Closed
rkingan opened this issue Jul 26, 2018 · 13 comments · Fixed by #35009
Closed

Add ability to generate graphs based on correlations of sequences #25933

rkingan opened this issue Jul 26, 2018 · 13 comments · Fixed by #35009

Comments

@rkingan
Copy link

rkingan commented Jul 26, 2018

Given a list of labeled real-valued vectors and a significance level alpha, generate a graph with vertices corresponding to the list's labels where two vertices are connected by an edge if the corresponding vectors have a correlation coefficient that is positive or negative at the specified significance level. An object creating the graph should support the plot method, which will plot the generated graph using a circular layout.

CC: @dcoudert

Component: graph theory

Keywords: sagedays@icerm

Author: Robert Kingan

Issue created by migration from https://trac.sagemath.org/ticket/25933

@rkingan rkingan added this to the sage-8.4 milestone Jul 26, 2018
@rkingan
Copy link
Author

rkingan commented Jul 26, 2018

comment:1

Added branch name

@rkingan
Copy link
Author

rkingan commented Jul 26, 2018

Branch: correlation-graph

@rkingan rkingan self-assigned this Jul 26, 2018
@mkoeppe mkoeppe removed this from the sage-8.4 milestone Dec 29, 2022
@Bruno-TT
Copy link
Mannequin

Bruno-TT mannequin commented Jan 2, 2023

Commit: abc6100

@Bruno-TT
Copy link
Mannequin

Bruno-TT mannequin commented Jan 2, 2023

comment:4

I've made a very bare-bones start on this but please tear it apart.

One note about my plot() implementation - the graph.plot function invokes the constructor at .__class__() about 10 methods deep (specifically, at the start of GenericGraph._subgraph_by_adding).

In the plot method I've put in a hack that works around it,

def plot(self):

    self.__class__=Graph
    p=self.plot()
    self.__class__=CorrelationGraph

    return p

but I think this is a symptom of a larger issue. Making assumptions about the arguments of the class constructor of a passed object feels like poor design, and we should be able to inherit the plot method with any class, without restrictions on the shape of said class' constructor.

If anyone can find a better workaround please let me know, this is the tidiest one I've found.


New commits:

abc6100initial implementation start

@Bruno-TT
Copy link
Mannequin

Bruno-TT mannequin commented Jan 2, 2023

Changed branch from correlation-graph to u/gh-Bruno-TT/correlation-graph

@Bruno-TT Bruno-TT mannequin added the s: needs info label Jan 2, 2023
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 2, 2023

Branch pushed to git repo; I updated commit sha1. New commits:

2798d32base test fix

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 2, 2023

Changed commit from abc6100 to 2798d32

@dcoudert
Copy link
Contributor

comment:7

Several comments:

  • graph generators are usually placed in src/sage/graphs/generators/ and made accessible as graphs.<TAB>
  • Your method must be commented. I'm not sure to understand the steps.
  • If you think the current design of the plot method must be improved, please open a dedicated ticket

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 15, 2023

Changed commit from 2798d32 to ee12eac

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 15, 2023

Branch pushed to git repo; I updated commit sha1. New commits:

ee12eacchanged class to generator method + tab completion

@dcoudert
Copy link
Contributor

comment:9

Better this way.

Note that the current doctest is not working since parameter include_anticorrelation is required. You should certainly set its default value and add doctests using it.

Coding style:

  • boolean_adjacency_matrix = (corrs>=alpha) -> boolean_adjacency_matrix = corrs >= alpha. I don't think you need the parathenses.
  • adjacency_matrix=Matrix(boolean_adjacency_matrix.astype(int)) -> adjacency_matrix = Matrix(boolean_adjacency_matrix.astype(int))

and of course add the documentation.

@Bruno-TT
Copy link
Contributor

Bruno-TT commented Feb 7, 2023

PR here: #35009

I've changed the above as per your suggestions David, and added some documentation

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 13, 2023

Removed branch from issue description; replaced by PR #35009

@vbraun vbraun closed this as completed in d73d7d7 May 22, 2023
@mkoeppe mkoeppe added this to the sage-10.1 milestone May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants