Unit test requirements (was Re: [Coin-discuss] OsiCbc)

[Ted Ralphs]

Alan King wrote:
> 
> Few comments below
>  >
>  > Also, the OsiSymSolverInterface unit test should work properly now.
>  > There was a bug in the copy constructor that is now fixed. Also, I had
>  > to comment out a few tests. SYMPHONY will not pass the common unit tests
>  > for two reasons:
>  >
>  > 1. We do not allow an infeasible solution to be loaded into the
>  > interface. I think this is a reasonable policy, since it is natural to
>  > assume that the currently loaded solution is feasible to the currently
>  > loaded problem. SYMPHONY has no mechanism for storing an infeasible
>  > solution. This unit test requires this. I'm not sure this is reasonable,
>  > but I'd be curious to hear other opinions.
> 
> If the user can modify the loaded solution, then it is reasonable to assume
> that it could be infeasible.

But the question is: should a solver be forced to store an infeasible 
solution if it does not make sense for it to do so? I understand that 
some solvers may be able to make use of infeasible solutions in some way 
and that it may be reasonable to allow a user to load an infeasible 
solution. However, SYMPHONY can only make use of feasible solutions and 
trying to load an infeasible solution results in an error. This seems to 
be a reasonable behavior. And yet, the unit test will fail in many 
places if the solver does not allow an infeasible solution to be loaded. 
This does not strike me as a behavior that should be required of every 
interface.

>  > 2. Pointers to rim vectors returned by methods such as getColLower()
>  > become invalid if the problem is modified. This is because SYMPHONY
>  > itself returns a copy of requested rim vectors to the interface (not a
>  > pointer to the internally stored data). The pointer to this copy is then
>  > cached locally in the interface. It is the pointer to this locally
>  > cached copy that is returned to the user. The locally cached arrays are
>  > deleted when certain problem data are modified and only recreated when a
>  > new pointer is requested. In particular, when any column bound is
>  > changed, the locally cached copies of both the upper and lower bound
>  > arrays are deleted. The common unit test requests a pointer to the
>  > bounds arrays, then modifies the problem and expects that pointer to
>  > remain valid. I'm not sure this is reasonable either. Pointers should
>  > probably only be required to remain valid while the problem data remains
>  > unchanged. I'd be curious to hear other opinions on this as well.
> 
> It is reasonable to require the user to suppose that a pointer to an
> internal array could be modified by its owning program during a subsequent
> call.  For the unit test, the bounds arrays returned in the subsequent
> call should be identical to the ones obtained previously.

I'm not sure whether you're agreeing with me or not. Are you saying the 
pointer returned should remain valid after a subsequent call or that the 
user should assume that such a pointer does not remain valid? To make it 
more concrete, here are some lines from the common unit test. First, 
pointers to arrays are requested:

     const double * cl = si2->getColLower();
     const double * cu = si2->getColUpper();

Later, these pointers are used as follows:

     assert( !eq(cl[3],1.2345) );
     symSi.setColLower( 3, 1.2345 );
     assert( eq(symSi.getColLower()[3],1.2345) );

     assert( !eq(cu[4],10.2345) );
     symSi.setColUpper( 4, 10.2345 );
     assert( eq(symSi.getColUpper()[4],10.2345) );

Note that the line

     assert( !eq(cl[3],1.2345) )

implicitly assumes that cl is still a valid pointer, even though there 
has been a subsequent modification to the problem data. I would say that 
this is not a valid assumption and that this should be replaced by

     assert( !eq(symSi.getColUpper()[4],10.2345) );

What do you think?

Ted
-- 
Dr. Ted Ralphs
Assistant Professor
Industrial and Systems Engineering
Lehigh University
(610)758-4784
tkralphs at lehigh.edu
www.lehigh.edu/~tkr2