HLR / DomiKnowS

37 stars 6 forks source link

Throwing Error on Double usage of variable names #379

Closed hfaghihi15 closed 1 year ago

hfaghihi15 commented 1 year ago

I think there should be an error thrown from the logical interface when the same name of the variable is repeated multiple times in the constraint. For instance, if e is defined before and now is not used in the path but rather as a new variable, then there should be an error specifying this scenario and stopping the program.

@auszok

auszok commented 1 year ago

When you use the same variable names it means that you want to talk about the same entity like "if x is c then x is b"

hfaghihi15 commented 1 year ago

When you use the same variable names it means that you want to talk about the same entity like "if x is c then x is b"

Hi Andrzej,

This is actually not enforced in our framework, the way we can do this is by using path, but if we say concept1('e') and concept2('e') the code you have considers two independent e variables and do not connect those with each other, we have to use concept1('e'),concept2(path('e')).

hfaghihi15 commented 1 year ago

When you use the same variable names it means that you want to talk about the same entity like "if x is c then x is b"

Hi Andrzej,

This is actually not enforced in our framework, the way we can do this is by using path, but if we say concept1('e') and concept2('e') the code you have considers two independent e variables and do not connect those with each other, we have to use concept1('e'),concept2(path('e')).

@auszok We should either enforce that they are the same or throw and error.

auszok commented 1 year ago

@hfaghihi15

Actually yes I create new variable for it but I do copy both dataNode candidates and ILP values from the variable, so the effect on lc processing should be the same. I am not sure why I create a new variable, yet.

https://github.com/HLR/DomiKnowS/blob/b2b21d1102d98b70dd8628814f20d3b2d408638a/domiknows/solver/gurobiILPOntSolver.py#L1016

hfaghihi15 commented 1 year ago

@hfaghihi15

Actually yes I create new variable for it but I do copy both dataNode candidates and ILP values from the variable, so the effect on lc processing should be the same. I am not sure why I create a new variable, yet.

https://github.com/HLR/DomiKnowS/blob/b2b21d1102d98b70dd8628814f20d3b2d408638a/domiknows/solver/gurobiILPOntSolver.py#L1016

I am not sure how this is processed, but I had a constraint which was not being enforced at all when I had defined e as part of combinationC and then used it like input_entity('e'), but then it got working by changing this to input_entity(path=('e')).

It actually took me a lot to degub and understand the error was there, so we have to either enforce that there two are exactly equivalent or give an error. In the candidateSelection code, you do not look at prior variables if the name is not mentioned in the path, instead you use datanode to retrieve the new candidates. @auszok

auszok commented 1 year ago

@hfaghihi15 I think I understand. Will it be possible that you commit this example to T5 and mark the constraint with active=Tested_Lc. I will use it to test the fix. Thank you.

hfaghihi15 commented 1 year ago

@hfaghihi15 I think I understand. Will it be possible that you commit this example to T5 and mark the constraint with active=Tested_Lc. I will use it to test the fix. Thank you.

I pushed the code, In line 109, now you see input_entity(path=('e')),, feel free to change this to input_entity('e'), to test your fix.

auszok commented 1 year ago

@hfaghihi15 It it is update the same variable used in the lc results in reusing the previously found for the same variable candidates. The T5 graph lc is updated to use it.