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

Integration with RobertGregg/FGES.jl #77

Closed
mschauer opened this issue Jan 17, 2023 · 9 comments
Closed

Integration with RobertGregg/FGES.jl #77

mschauer opened this issue Jan 17, 2023 · 9 comments

Comments

@mschauer
Copy link
Owner

@RobertGregg I create this issue to see how can integrate your FGES algorithm implementation https://github.com/RobertGregg/FGES.jl

@mschauer
Copy link
Owner Author

mschauer commented Jan 17, 2023

Found two functions which do about the same thing:

https://github.com/RobertGregg/FGES.jl/blob/3d396135f841d5e2f209eacb4d001194f5805fb9/src/graphStructure.jl#L234

function has_a_path(g::AbstractGraph{T}, U::Vector, V::Vector,

That should help getting acquainted with the different interfaces (CausalInference uses directed Graphs.jl, with PDAG represented as DAGs with both the forward and the backward edge added, and FGES uses their own graph structure)

@mschauer
Copy link
Owner Author

@samurai-kant Might be interesting for you as well.

@RobertGregg
Copy link
Contributor

Fantastic thank you for starting this thread.

Switching over to Graphs.jl using a DiGraph should be really easy because I borrowed a lot of naming conventions from Graphs.jl. I think at the time I didn't realize one edge could have multiple directions in a DiGraph.

It does look like has_a_path and isblocked are very similar just with the logic reversed. Does has_a_path look at semi-directed paths (e.g. would src → x₁ - x₂ → dest be valid?).

Would it make sense to put some of the functions used by multiple algorithms into a common file? For example, the has_a_path and is_parent functions can be used by both fges and fci. Maybe the "graph.jl" file or a new one?

Do you have a specific naming convention for functions/structures? It looks like you generally use snake_case which I can adopt.

@mschauer
Copy link
Owner Author

Does has_a_path look at semi-directed paths (e.g. would src → x₁ - x₂ → dest be valid?).

Yes, as undirected is represented by both edges just
following forward edges just works

@mschauer
Copy link
Owner Author

  1. Yes, I am happy with the experience of just using digraphs from Graphs.jl and would recommmend that, I am also happy to structure the code more it has grown organically and could be refactored. And I loosely follow the Julia style of snake_case with underscore omitted sometimes for readability, I.e “getindex”

@RobertGregg
Copy link
Contributor

So I forked the repository and got FGES running without any errors. However, I'm not getting the same results as before from testing. I think it might be due to edges(graph) iterating over both x => y and y => x when x-y is undirected. It could also be something silly like a typo 😵

Just out curiosity, what does "Package Extensions" do? I noticed it in the main CausalInference.jl module

@mschauer
Copy link
Owner Author

edges(graph) iterating over both x => y and y => x when x-y is undirected. I

Yes, exactly!

Just out curiosity, what does "Package Extensions" do?

The code to plot graphs depends on the plotting package, we use PackageExtension to load the right code for the users plotting package.

@mschauer
Copy link
Owner Author

mschauer commented Jul 4, 2023

I am still eager to pick that up!

@sumantrak
Copy link
Collaborator

I am happy to contribute, but my progress will be a bit slow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants