FenTechSolutions / CausalDiscoveryToolbox

Package for causal inference in graphs and in the pairwise settings. Tools for graph structure recovery and dependencies are included.
https://fentechsolutions.github.io/CausalDiscoveryToolbox/html/index.html
MIT License
1.12k stars 197 forks source link

[BUG] GES + csv file is read wrong in R template #89

Closed juangamella closed 3 years ago

juangamella commented 3 years ago

Hi folks,

Looking at the current master branch of the repository, i.e. commit 31809d80098846bafcab08ebb09fd5e0ceb68fba.

In line 26 of the GES R template:

cdt/causality/graph/R_templates/ges.R

you do not specify that the csv has no header:

dataset <- read.csv(file='{FOLDER}{FILE}', sep=",");

This causes the first row to be lost. The effect is quite subtle. I only noticed because I am reimplementing GES in another language and the likelihood scores between my implementation and that of pcalg did not match. I've spent several days searching for numerical instabilities until I found this. No hard feelings, I'm glad it's over now :)

Best,

Juan

PS: My R version is3.6.1 and my pcalg version is 2.7.0 although I doubt that matters much.

diviyank commented 3 years ago

Hello, Huge thanks for noticing this ! I'll be fixing this asap! Out of curiosity, in what language are you reimplementing GES ?

Best, Diviyan

juangamella commented 3 years ago

Hi Diviyan,

Thanks for your answer!

In python, so it can also be modified easily by people who don't know R (like myself).

Glad to be of help :)

Juan

On Mon, Dec 28, 2020, 14:01 Diviyan Kalainathan notifications@github.com wrote:

Hello, Huge thanks for noticing this ! I'll be fixing this asap! Out of curiosity, in what language are you reimplementing GES ?

Best, Diviyan

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/FenTechSolutions/CausalDiscoveryToolbox/issues/89#issuecomment-751704953, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACOTSKKUCF6FVL7OZIIGOY3SXB6UDANCNFSM4VBP5CRA .

diviyank commented 3 years ago

This should be fixed, and pushed for the next release :) thanks again !