Feature/feasibility

[jlalbers]

@arcondello adds a feasible() method to the Model class to make it easier for users to check the feasibility of a model state without manually checking the state of each constraint symbol. Addresses #82

Current work will build, but throws a segfault when I try to actually call the method in Python.

[alexzucca90]

I suspect it seg-faults because you need to initialize the state before calling the feasible method. This should be the cause if in your test, when calling it, you set a state and then called the feasible method right after
So you can add some python tests! :)

[alexzucca90]

Sorry, I think I was unclear earlier. I meant here

self._graph.initialize_state(self.states._states[index])
return self._graph.feasible(self.states._states[index])

[jlalbers]

Ahh gotcha. Sorry, just my unfamiliarity with the codebase. Anything after initializing the state I'd need to clean up?

[alexzucca90]

I think that should be it.. there are two things worth remembering here. The C++ method initialize_state() initialize all nodes in the graph (eagerly) to their value given the decision variables. If you were to check every single constraint from the python side, then it would be lazily from checking each constraint. Worth keeping this in mind in case one deals with models with very deep graphs on the objective side and simple constraints on the decisions (kinda a worst case scenario)

[jlalbers]

I'll try doing some performance analysis and see if there's a significant cost with the eager initialization. Also could test with common problem types (i.e. models created by the generator functions) and see if it makes any difference

[arcondello]

The more I think about it, the more I think we should not expose the C++ method and just do a pure python

    def feasible(self, int index = 0):
        """Check the feasibility of the state at the input index.
        Args:
            index: index of the state to check for feasibility.
        Returns:
            Feasibility of the state.
        """
        return all(sym.state(index) for sym in self.iter_constraints())

sure there will be a minor performance hit. But this covers all of the edge cases (unitilized states, unresolved states, out of bounds indexing, no constraints, etc.)

If we later find it to be an issue, we can do the more performant thing. But IMO it won't be that much better after we do all of the edge case handling.

[jlalbers]

There was a bug when checking the index against the state size (used the state_size method instead of states.size. However, I'm still getting a segfault when calling feasible with index 0 using an initialized state, i.e. the following:

...
>>> i = model.integer()
>>> c = model.constant(5)
>>> model.add_constraint(i <= c)
>>> model.feasible(0)
...
ValueError: index 0 is out of range # Expected behavior
>>> model.states.resize(1)
>>> i.set_state(0, 1) # Feasible state
>>> model.feasible(0)
Process finished with exit code 139 (interrupted by signal 11:SIGSEGV)

Could it be something to do with the State type alias used by Cython being different from the one used in the Graph C++ implementation? Could throw a segfault if the type aliases represent different objects and Graph::feasible tries to illegally access a member that ends up being at the wrong memory offset for a different object type

[alexzucca90]

maybe we can also return True if number of constraints is 0

[arcondello]

The more I think about it, the more I think we should not expose the C++ method and just do a pure python

    def feasible(self, int index = 0):
        """Check the feasibility of the state at the input index.
        Args:
            index: index of the state to check for feasibility.
        Returns:
            Feasibility of the state.
        """
        return all(sym.state(index) for sym in self.iter_constraints())

sure there will be a minor performance hit. But this covers all of the edge cases (unitilized states, unresolved states, out of bounds indexing, no constraints, etc.)

If we later find it to be an issue, we can do the more performant thing. But IMO it won't be that much better after we do all of the edge case handling.

[arcondello]

LGTM, will merge when/if the CI passes

[alexzucca90]

LGTM