coin-or / python-mip

Python-MIP: collection of Python tools for the modeling and solution of Mixed-Integer Linear programs
Eclipse Public License 2.0
525 stars 92 forks source link

Error when removing constraints, constr_by_name does not return the right constraint #99

Closed pabloazurduy closed 4 years ago

pabloazurduy commented 4 years ago

Hi I was trying to iterative eliminate constraints and I found a rare behavior, maybe I'm doing something wrong. but when I extract a constraint using constr_by_name I'm receiving the wrong constraint

This is the code to reproduce the error:


from mip import Model
import numpy as np 
from mip.constants import CONTINUOUS
import logging

logger = logging.getLogger(__name__)

def build_infeasible_cont_model(num_constraints:int = 10, num_infeasible_sets:int = 20) -> Model:
    # build an infeasible model, based on many redundant constraints 
    mdl = Model(name='infeasible_model_continuous')
    var = mdl.add_var(name='var', var_type=CONTINUOUS, lb=-1000, ub=1000)

    for idx,rand_constraint in enumerate(np.linspace(1,1000,num_constraints)):
        crt = mdl.add_constr(var>=rand_constraint, name='lower_bound_{}'.format(idx))
        logger.debug('added {} to the model'.format(crt))

    num_constraint_inf = int(num_infeasible_sets/num_constraints)
    for idx,rand_constraint in enumerate(np.linspace(-1000,-1,num_constraint_inf)):
        crt = mdl.add_constr(var<=rand_constraint, name='upper_bound_{}'.format(idx))
        logger.debug('added {} to the model'.format(crt))

    mdl.emphasis = 1 # feasibility
    mdl.preprocess = 1 # -1  automatic, 0  off, 1  on.
    # mdl.pump_passes TODO configure to feasibility emphasis 
    return mdl

import sys
if __name__ == "__main__":

    # logger config
    handler = logging.StreamHandler(sys.stdout)
    logger.setLevel(logging.DEBUG)
    logger.addHandler(handler)

    # model experiment
    model = build_infeasible_cont_model()
    logger.debug(model.status)
    model.optimize()
    logger.debug(model.status)

    model_copy =  model.copy()
    for inc_crt in model.constrs:
        logger.debug('removing temporally {}'.format(inc_crt.name))
        aux_model_inc_crt = model_copy.constr_by_name(inc_crt.name)
        logger.debug('removing temporally {}'.format(aux_model_inc_crt.name))
        model_copy.remove(aux_model_inc_crt)

The logger should return something like

removing temporally lower_bound_0 removing temporally lower_bound_0 removing temporally lower_bound_1 removing temporally lower_bound_1 removing temporally lower_bound_2 removing temporally lower_bound_2 removing temporally lower_bound_3 removing temporally lower_bound_3 removing temporally lower_bound_4 removing temporally lower_bound_4

but Instead is returning:

removing temporally lower_bound_0 removing temporally lower_bound_0 removing temporally lower_bound_1 removing temporally lower_bound_2 removing temporally lower_bound_2 Traceback (most recent call last): File "/home/pablo/github/python-mip-infeasibility/get_name_error.py", line 47, in logger.debug('removing temporally {}'.format(aux_model_inc_crt.name)) AttributeError: 'NoneType' object has no attribute 'name'

It seams like when Im removing objects the constr_by_name method is not working properly, because I´m sending the right str but it's returning a different constraint.

how can I correctly identify a constraint in a mutable model ? is there a unique idx that does not change when I remove a constraint ? why the method constr_by_name is returning the wrong constraint ?

pabloazurduy commented 4 years ago

a hacky solution finding manually the new crt.idx of your name constraint

        aux_model_inc_crt_idx = [crt.name for crt in model_copy.constrs].index(inc_crt.name)
        aux_model_inc_crt = model_copy.constrs[aux_model_inc_crt_idx]

that will behave correctly

removing temporally lower_bound_0 removing temporally lower_bound_0 removing temporally lower_bound_1 removing temporally lower_bound_1 removing temporally lower_bound_2 removing temporally lower_bound_2 ...

This seems like a problem with the idx re-mapping after a Model.remove or, we can just replace the Model.constr_by_name() to actually get the correct crt.

h-g-s commented 4 years ago

Hi @pabloazurduy , just fixed a bug in the CBC C interface, to update indexes after deleting constraints so that constr_by_name works (I'll push later). Anyway, not sure if your example is just to show the bug or you are actually removing constraints this way, but if you plan to remove several constraints at once it is better, you can pass a list of constraints to remove. I also think that it is dangerous in general to iterate and delete elements of the set being iterated, isn't ?

pabloazurduy commented 4 years ago

Hi @h-g-s many thanks! about your questions:

not sure if your example is just to show the bug or you are actually removing constraints this way

I need to remove them in a for loop but after each iteration check the feasibility (I was not looking to remove them as a batch), but thanks for the heads-up!

I also think that it is dangerous in general to iterate and delete elements of the set being iterated, isn't ?

yes your right, so that's why I made a copy of the model and remove the constraints from the copy and not from the original model_copy = model.copy(). At the begging, I was trying to iterate over a copy of all constraints all_constrains = deepcopy(model.constrs ) or all_constrains = model.constrs.copy() but none of these methods works, I realized that the classConstrList actually is tight with the Model object, so I ended up using an auxiliary model with my constraint-subset. In the same topic, I wanted to copy vars and constraints from one model to another, using something like mcopy.vars = moriginal.vars.copy() but It didn't work, so I ended up looping and adding them one by one

for var in moriginal.vars:
    mcopy.add_var(var.name,var.lb....)

Is there a more efficient way to do that?. Again, many thanks for your help !!

h-g-s commented 4 years ago

Hi @h-g-s many thanks! about your questions:

not sure if your example is just to show the bug or you are actually removing constraints this way

I need to remove them in a for loop but after each iteration check the feasibility (I was not looking to remove them as a batch), but thanks for the heads-up! Hmm, ok, yes, in this case it need to be removed one by one. I think that something like: while model.constrs: model.remove(model.constrs[-1]) # or any other model.optimize() would be more clear

I also think that it is dangerous in general to iterate and delete elements of the set being iterated, isn't ?

yes your right, so that's why I made a copy of the model and remove the constraints from the copy and not from the original model_copy = model.copy(). At the begging, I was trying to iterate over a copy of all constraints all_constrains = deepcopy(model.constrs ) or all_constrains = model.constrs.copy() but none of these methods works, I realized that the classConstrList actually is tight with the Model object, so I ended up using an auxiliary model with my constraint-subset. In the same topic, I wanted to copy vars and constraints from one model to another, using something like mcopy.vars = moriginal.vars.copy() but It didn't work, so I ended up looping and adding them one by one

understood now

for var in moriginal.vars:
    mcopy.add_var(var.name,var.lb....)

Is there a more efficient way to do that?. Again, many thanks for your help !!

h-g-s commented 4 years ago

w.r.t. adding vars one by one, it should be relatively fast, since model updates like that are cached in the C interface and then performed in batch.

pabloazurduy commented 4 years ago

solved on the last version 1.9.3