support OR operator in binary evaluate_bounds
#15716
Conversation
github-actions
bot
added
logical-expr
|
The failing tests were added in #14189 and #14735, both which talk about supporting OR as a future possibility. Ping @berkaysynnada as the reviewer / approver of both those PRs, does this PR look ok to you? If so I will update those tests so that CI is green here. |
|
Thank you @davidhewitt. I will review this today |
|
Failing tests should be fixed after taking a merge from main |
|
Thanks, I will probably get around to fixing this up on Tuesday 👍 |
davidhewitt
force-pushed
the
dh/evaluate-bounds-or
branch
from
161775e to
f5092b6
Compare
There was a problem hiding this comment.
Sorry for the delay @berkaysynnada. This should be good to review in full now.
| // Special case: true OR could in principle be propagated by 3 interval sets, | ||
| // (i.e. left true, or right true, or both true) however we do not support this yet. | ||
| if node_interval == &Interval::CERTAINLY_TRUE | ||
| && self.graph[node] | ||
| .expr | ||
| .as_any() | ||
| .downcast_ref::<BinaryExpr>() | ||
| .is_some_and(|expr| expr.op() == &Operator::Or) | ||
| { | ||
| return not_impl_err!("OR operator cannot yet propagate true intervals"); | ||
| } |
There was a problem hiding this comment.
I had to add this to keep the invalid-analysis tests failing. I could move this inside the implementation of PhysicalExpr for BinaryExpr, however that case currently returns Ok(Some(vec![])) and it wasn't clear to me if it was ok to change that case to be failing.
There was a problem hiding this comment.
Rather than error, this case should go to PropagationResult::CannotPropagate, if any child is true, or PropagationResult::Infeasible if both of them are false
|
Thanks @davidhewitt and @berkaysynnada |
|
Thanks! |
* support OR operator in binary `evaluate_bounds` * fixup tests
Which issue does this PR close?
apply_operatordoes not support OR #15715 .Rationale for this change
Seems like this should have worked as per the commentary in the issue.
What changes are included in this PR?
ORcase toapply_operatorevaluate_boundsAre these changes tested?
Yes
Are there any user-facing changes?
Yes (public function now supports additional operator)