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

fix skipping over values in constraint implementation #380

Conversation

matzemathics
Copy link
Collaborator

No description provided.

@matzemathics matzemathics requested a review from aannleax October 18, 2023 11:21
@matzemathics matzemathics self-assigned this Oct 18, 2023
@matzemathics matzemathics added bug Something isn't working physical physical layer labels Oct 18, 2023
@matzemathics
Copy link
Collaborator Author

@mmarx mmarx added this to the Release 0.4.0 milestone Oct 18, 2023
@aannleax
Copy link
Member

aannleax commented Oct 18, 2023

I'm not sure what we want actually. For something like R(?z) :- A(?x, ?y), ?z = ?x / ?y, we want to skip all results with ?y = 0. For constraints R(?x, ?y) :- A(?x, ?y), ?x / ?y > 10 I'm not sure but I think we also want to skip the rows (so treat it as a if the constraint was not satisfied).

To do this the function check_condition should probably just have the return type bool and go into State::After if there is an undefined expression.

satisfy_lower_bound should probably return nothing and loop while tree.evaluate returns None

check_upper_bound could stay as before, but the code that uses it should loop until it returns either true or false

@aannleax
Copy link
Member

I can also do the fixes, if you want

@matzemathics
Copy link
Collaborator Author

I can also do the fixes, if you want

Yes feel free to pick this up, I'm not sure I understand all your remarks.

Skipping rows, where division by zero occurs is expected for me, the sole problem is that it also skipped non-divide-by-zero-rows that are in the same group as a divide-by-zero-row, which seems unexpected to me.

@aannleax
Copy link
Member

Ok, I think it makes sense now

@matzemathics
Copy link
Collaborator Author

Ok, I think it makes sense now

Nice, I will try and add some more test cases then we can get this merged

@matzemathics matzemathics force-pushed the 379-restrict-column-misses-elements-when-encoutering-undefined-operations branch from bd33dd4 to 28a4fb4 Compare October 18, 2023 15:21
@matzemathics matzemathics requested a review from mmarx October 20, 2023 05:55
@matzemathics
Copy link
Collaborator Author

matzemathics commented Oct 20, 2023

@aannleax @mmarx anything still missing? otherwise I'd suggest we get this merged.

@aannleax aannleax merged commit 3d7de11 into main Oct 20, 2023
@matzemathics matzemathics deleted the 379-restrict-column-misses-elements-when-encoutering-undefined-operations branch October 26, 2023 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working physical physical layer
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Restrict column misses elements, when encoutering undefined operations
3 participants