bd2kccd / causal-cmd

16 stars 8 forks source link

IMaGES not using the penalty parameter #51

Closed rubenSaro closed 3 years ago

rubenSaro commented 4 years ago

@kvb2univpitt in the 1.1.3 release, the fGES algorithm now works ok, but the IMaGES algorithm still output an error when input the penaltyDiscount parameter

kvb2univpitt commented 4 years ago

@rubenSaro That's strange. I'll take a look at it today. Thanks.

kvb2univpitt commented 4 years ago

@rubenSaro Both IMaGES Discrete and IMaGES Continuous don't take either test or score. Therefore, there's no penaltyDiscount parameter for it. See screenshots of tetrad-gui below: img_cont

img_disc

rubenSaro commented 4 years ago

@kvb2univpitt @espinoj That is weird. Those algorithms are based on fGES so they should take as a parameter the penalty discount. My take then is that the GUI also have a bug.

kvb2univpitt commented 4 years ago

@rubenSaro @espinoj @jdramsey IMaGES Discrete uses ImagesBDeu which is using BDeu score. However, parameters for sample prior and structure prior are not exposed as it should be. You're right. There's a bug and I know exactly why. The method getParameters() of the algorithms are using the old way of getting parameters. Thus, it's only getting the parameters for the algorithm and not the score, as shown below: List<String> parameters = new Fges(new BdeuScore(), false).getParameters();

The correct way would be this:

List<String> parameters = new LinkedList<>();
parameters.addAll((new Fges()).getParameters());
parameters.addAll((new BdeuScore()).getParameters());

We need to fix this for all algorithms.

jdramsey commented 4 years ago

I think the issue here is that those parameters need to be given explicitly for those algorithms; there was an issue with allowing those algorithms to take an arbitrary score, so the score parameters had to be given as algorithm parameters. I think if you just add those parameters to the algorithms they should be extraordinarily happy.

kvb2univpitt commented 4 years ago

@rubenSaro This is now fixed in tetrad 6.8.0-SNAPSHOT. The causal-cmd in the development branch uses tetrad 6.8.0-SNAPSHOT. You can compile causal-cmd from the development branch and use it. You should penaltyDiscount parameter for IMaGES.

rubenSaro commented 4 years ago

thanks Kevin, I will check it out.

ruben

On Sat, Nov 9, 2019, 2:53 PM Kevin Bui notifications@github.com wrote:

@rubenSaro https://github.com/rubenSaro This is now fixed in tetrad 6.8.0-SNAPSHOT. The causal-cmd in the development branch uses tetrad 6.8.0-SNAPSHOT. You can compile causal-cmd from the development branch and use it. You should penaltyDiscount parameter for IMaGES.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bd2kccd/causal-cmd/issues/51?email_source=notifications&email_token=AJUZSFMQ4XNZS4YSOCENZATQS4IKRA5CNFSM4JIYY5GKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDUNYVI#issuecomment-552131669, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJUZSFPWKO4B56X3ZL3JZTTQS4IKRANCNFSM4JIYY5GA .

espinoj commented 4 years ago

@rubenSaro we also have latest downloadable compiled versions at https://cloud.ccd.pitt.edu/latest-dev-tetrad.html

espinoj commented 4 years ago

@rubenSaro for causal cmd too! https://cloud.ccd.pitt.edu/latest-dev-causalcmd.html

kvb2univpitt commented 4 years ago

@espinoj The link above for causal-cmd does not have the latest changes. It's using tetrad 6.7.0, which doesn't have the changes I made today.

espinoj commented 4 years ago

@kvb2univpitt Ok. I see. We must have changed something since the auto snapshots on our maven repo don't seem to be up to date. I'll fix.