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

[Brady Hunsaker]

Without commenting on the specific issues below, I'd like to suggest 
that we form a group of interested OSI developers to try to more 
carefully specify expected OSI behavior and design the unit test 
accordingly.  The issues in this email need to be resolved, as do some 
that have come up in the past.

As maintainer of OsiGlpk I have never been too careful about the unit 
test for these reasons.  If we put some thought into clarifying the 
behavior we want, then we can write it down for developers and adjust 
the unit test accordingly so that it is more useful and all interfaces 
regularly pass the unit test.  I think that will help OSI a great deal.

What do you think of creating such written guidelines for OSI behavior? 
  I'm happy to be part of the group, though I would prefer that someone 
with broader OSI experience take the lead.

Brady

Ted Ralphs wrote:
> 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


-- 
Brady Hunsaker
Assistant Professor
Industrial Engineering
University of Pittsburgh
http://www.engr.pitt.edu/hunsaker/