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

Unit propagation on monomials #6905

Merged
merged 82 commits into from
Oct 9, 2023
Merged

Unit propagation on monomials #6905

merged 82 commits into from
Oct 9, 2023

Conversation

levnach
Copy link
Contributor

@levnach levnach commented Sep 15, 2023

No description provided.


T& m_imp;
vector<implied_bound> m_ibounds;
std::vector<implied_bound, my_allocator<implied_bound>> m_ibounds;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will be moving this as a general definition to util/vector in a separate pull request.

@@ -86,76 +122,248 @@ class lp_bound_propagator {
public:
lp_bound_propagator(T& imp) : m_imp(imp) {}

const vector<implied_bound>& ibounds() const { return m_ibounds; }
const std::vector<implied_bound, my_allocator<implied_bound>>& ibounds() const { return m_ibounds; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is why it deserves a stand-alone typedef or sub-class.

return;

m_bp.init();
propagate_bounds_for_touched_monomials();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see where the bounds found by nlp monomials are propagated to LP. The bounds are "just" imposed on existing literals. We didn't have a way to propagate the resulting bounds on to the LP tableau. The problem, of course, is that this kind of bounds propagation is fragile with respect to convergence. So it has to be throttled or has to use detection of propagation loops. Dealing with this properly requires more than a simple code change.

levnach and others added 3 commits September 19, 2023 10:56
Signed-off-by: Lev Nachmanson <levnach@hotmail.com>
Signed-off-by: Nikolaj Bjorner <nbjorner@microsoft.com>
Signed-off-by: Nikolaj Bjorner <nbjorner@microsoft.com>
@@ -511,6 +511,7 @@ bool theory_arith<Ext>::propagate_nl_downward(expr * n, var_power_pair const& p)
*/
template<typename Ext>
bool theory_arith<Ext>::propagate_nl_bounds(expr * m) {
return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should not be part of pull request

const auto& vars = m_emons[monic_var].vars();
if (!is_linear(vars, zero_var, non_fixed))
return;
if (throttle)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks really strange to me: what is the case where you don't want to throttle?
Once a monomial is linear it is either fixed or has a single non-fixed variable. The single non-fixed variable times the values of the fixed variables is equal to the variable of the monomial.
The monomial is no longer representing a non-linear sub-term. Strengthening bounds doesn't make it more linear. Whatever information comes from stronger bounds at this point should be captured by what got propagated.

for (auto v : vars)
if (v != non_fixed) {
k *= val(v);
if (k.is_big()) return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you throttle on is_big()?
There is no such throttling for arith.solver=2

bool is_strict;
auto& lps = lra;

if (lower_bound_is_available(non_fixed)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this so involved and yet not adding equalities to the tableau?
The equality should be that monic_var = k*non_fixed explained by dependencies for the fixed variables in vars.

}
}

bool core::lower_bound_is_available(unsigned j) const {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one now has two implementations.
Neither appear to be the right place, which would be lar_solver since bounds are maintained there. This is not specific to non-linear

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how are these functions related to column_has_upper/lower_bound in lp_core_solver_base.h, and re-exposed from lar_solver?

@levnach levnach merged commit 75e29b2 into master Oct 9, 2023
24 checks passed
@levnach levnach deleted the unit_prop_on_monomials branch October 9, 2023 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants