Adding FGES #78

RobertGregg · 2023-02-01T00:40:24Z

So it looks like I've got FGES working with the DiGraphs from Graphs.jl. I've also simplified some parts of the package I wrote:

Got rid of the message() function as that was specific to running the package on a cluster
Using good old X \ y to perform linear regression
Changed the debug keyword with verbose to match the other algorithms

I added a test for fges (copying the test from pc2.jl) and it passed. Let me know if you want me to add other tests.

I also added two dependencies Memoization and LRUCache. FGES performs A LOT of linear regressions and often performs the same one more than once. These two packages basically keep a dictionary of inputs and outputs that it checks before running the score function.

Let me know what you think!

mschauer · 2023-02-01T07:53:50Z

src/fges.jl

+
+"""
+    fges(data; penalty = 1.0, verbose=false)
+Compute a causal graph for the given observed data.


Suggested change

Compute a causal graph for the given observed data.

Compute a causal graph for the given observed data.

mschauer · 2023-02-01T07:54:55Z

test/fges.jl

+d = Normal()
+p = 0.01


Suggested change

d = Normal()

p = 0.01

unused?

mschauer · 2023-02-01T07:55:51Z

src/fges_helper.jl

+    neighborList = Int64[]
+
+    #loop through all vertices except for x
+    for vᵢ in Iterators.filter(!isequal(x),vertices(g))


Suggested change

for vᵢ in Iterators.filter(!isequal(x),vertices(g))

for vᵢ in Iterators.filter(!isequal(x), vertices(g))

mschauer · 2023-02-01T07:57:38Z

src/fges_helper.jl

+allpairs(v) = Iterators.filter(i -> isless(i...), Iterators.product(v,v))
+
+#Get every undirected edge in the entire graph g
+allundirected(g) = [Edge(nodePair) for nodePair in allpairs(vertices(g)) if isundirected(g,nodePair)]


Make it a generator?

Do you mean something like this?

Suggested change

allundirected(g) = [Edge(nodePair) for nodePair in allpairs(vertices(g)) if isundirected(g,nodePair)]

allundirected(g) = (Edge(nodePair) for nodePair in allpairs(vertices(g)) if isundirected(g,nodePair))

mschauer · 2023-02-01T07:58:38Z

src/fges_helper.jl

+# Neighborhood functions 
+####################################################################
+
+#Should these be non-allocating iterators? 


Aren't they?

I think they weren't originally, but I changed it and forgot to remove the comment.

mschauer · 2023-02-01T07:58:49Z

src/fges_helper.jl

+####################################################################
+
+"""
+    orientedge!(g, x, y)


Suggested change

orientedge!(g, x, y)

orientedge!(g, x, y)

mschauer · 2023-02-01T07:59:01Z

src/fges_helper.jl

+
+#Do the nodesRemoved block all semi-directed paths between src and dest?
+"""
+    isblocked(g, src, dest, nodesRemoved)


Suggested change

isblocked(g, src, dest, nodesRemoved)

isblocked(g, src, dest, nodesRemoved)

mschauer · 2023-02-01T08:02:46Z

Very nice, no problem with the additional dependencies. One could think of getting rid of camel case in function names findNextEquivClass! => find_next_equiv_class! but that's a bit busy-work. I left some comments!

mschauer · 2023-02-01T08:04:06Z

src/fges.jl

+end  
+
+#Simple structure to hold the current edge, a subset of neighbors, and a score change
+Base.@kwdef mutable struct Step{A,B}


Suggested change

Base.@kwdef mutable struct Step{A,B}

Base.@kwdef mutable struct FESStep{A,B}

This should have a more specific name

mschauer · 2023-02-01T08:04:58Z

src/fges.jl

+
+#Simple structure to hold the current edge, a subset of neighbors, and a score change
+Base.@kwdef mutable struct Step{A,B}
+    edge::Edge = Edge(1,2)


Could be that this should be SimpleEdge as Edge is an abstract type. Is this struct performance critical / used in a tight loop?

Very good point. We should change this to a concrete type

mschauer · 2023-02-01T08:05:31Z

src/fges.jl

+####################################################################
+
+"""
+    fges(data; penalty = 1.0, verbose=false)


Suggested change

fges(data; penalty = 1.0, verbose=false)

fges(data; penalty = 1.0, verbose=false)

I am adding these spaces after the function signature in the doc string

mschauer · 2023-02-01T10:13:05Z

You committed the manifest and a settings file by accident, I think this might cause the test failure.

mschauer · 2023-05-18T06:56:50Z

What are your thoughts on this?

RobertGregg · 2023-05-18T20:49:31Z

Ah apologies, I completely dropped the ball on this. I'll look at your comments this weekend in more detail, but overall they seem like good/minor additions. I'll make sure the manifest file doesn't get committed and check out the Edge() struct. That might have been leftover from when I was converting to using Graphs.jl

Not needed for FGES

RobertGregg

All the suggestions seem good. I've only done a pull request once before so sorry if anything looks weird. I'll try and commit the suggestions and remove the manifest file to see if things check out.

RobertGregg · 2023-05-23T00:22:19Z

src/fges.jl

+
+#Simple structure to hold the current edge, a subset of neighbors, and a score change
+Base.@kwdef mutable struct Step{A,B}
+    edge::Edge = Edge(1,2)


Very good point. We should change this to a concrete type

RobertGregg · 2023-05-23T00:27:09Z

src/fges_helper.jl

+allpairs(v) = Iterators.filter(i -> isless(i...), Iterators.product(v,v))
+
+#Get every undirected edge in the entire graph g
+allundirected(g) = [Edge(nodePair) for nodePair in allpairs(vertices(g)) if isundirected(g,nodePair)]


Do you mean something like this?

Suggested change

allundirected(g) = [Edge(nodePair) for nodePair in allpairs(vertices(g)) if isundirected(g,nodePair)]

allundirected(g) = (Edge(nodePair) for nodePair in allpairs(vertices(g)) if isundirected(g,nodePair))

mschauer · 2023-05-23T15:16:55Z

I think you forgot to add the new test to the runtest file.

mschauer · 2023-05-24T04:08:47Z

Ps: If it is easier, just make a new PR.

mschauer · 2023-07-22T15:28:09Z

Continued in #92

RobertGregg added 3 commits January 26, 2023 14:00

addition for FGES

6d26a0f

Fixed meekrules

0af258f

Added PC2 test to fges

bda9942

mschauer reviewed Feb 1, 2023

View reviewed changes

mschauer mentioned this pull request Feb 1, 2023

[WIP]Adding ges #71

Closed

Delete settings.json

d60f488

Not needed for FGES

RobertGregg commented May 23, 2023

View reviewed changes

RobertGregg and others added 2 commits May 22, 2023 17:36

Merge branch 'mschauer:master' into master

632732a

Delete Manifest.toml

b532f0f

Merge branch 'fges' into master

86f68e0

mschauer mentioned this pull request Jul 22, 2023

GES continuation #92

Merged

mschauer closed this Jul 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding FGES #78

Adding FGES #78

RobertGregg commented Feb 1, 2023

mschauer Feb 1, 2023

mschauer Feb 1, 2023

mschauer Feb 1, 2023

mschauer Feb 1, 2023

mschauer Feb 1, 2023

RobertGregg May 23, 2023

mschauer May 23, 2023

mschauer Feb 1, 2023

RobertGregg May 23, 2023

mschauer Feb 1, 2023

mschauer Feb 1, 2023

mschauer commented Feb 1, 2023

mschauer Feb 1, 2023

mschauer Feb 1, 2023

RobertGregg May 23, 2023

mschauer Feb 1, 2023

mschauer commented Feb 1, 2023

mschauer commented May 18, 2023

RobertGregg commented May 18, 2023

RobertGregg left a comment

RobertGregg May 23, 2023

RobertGregg May 23, 2023

mschauer commented May 23, 2023

mschauer commented May 24, 2023

mschauer commented Jul 22, 2023

	Compute a causal graph for the given observed data.

	Compute a causal graph for the given observed data.

	for vᵢ in Iterators.filter(!isequal(x),vertices(g))
	for vᵢ in Iterators.filter(!isequal(x), vertices(g))

	allundirected(g) = [Edge(nodePair) for nodePair in allpairs(vertices(g)) if isundirected(g,nodePair)]
	allundirected(g) = (Edge(nodePair) for nodePair in allpairs(vertices(g)) if isundirected(g,nodePair))

	isblocked(g, src, dest, nodesRemoved)
	isblocked(g, src, dest, nodesRemoved)

	Base.@kwdef mutable struct Step{A,B}
	Base.@kwdef mutable struct FESStep{A,B}

	fges(data; penalty = 1.0, verbose=false)
	fges(data; penalty = 1.0, verbose=false)

Adding FGES #78

Adding FGES #78

Conversation

RobertGregg commented Feb 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mschauer commented Feb 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mschauer commented Feb 1, 2023

mschauer commented May 18, 2023

RobertGregg commented May 18, 2023

RobertGregg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mschauer commented May 23, 2023

mschauer commented May 24, 2023

mschauer commented Jul 22, 2023