[Scip] Design choices

[michael.winkler@zib.de]

Hi Stefan,

>> > 1. Naming: why is it e.g. "SCIPsetObjlimit" with a lower case L?
>> > "objective" and "limit" are two words which should use camel case as
>> > your general naming convention implies.
>>
>> You are right - the naming convention would suggest SCIPsetObjLimit(),
>> which is probably better. However, we considered objlimit as a single
>> word here (this also happens for several other names) and I do not think
>> it makes sense to change it.
>>
>
> Well, consistency would be the keyword here. And it wouldn't harm if
> SCIPsetObjlimit (and similar methods) are deprecated but still usable.
>

This might be true, but even if we would put it on our todo list, it
probably will be positioned quite at the end, I think.

>>
>> > 2. Why is "SCIPallocMemory" a macro while not written in upper case
>> > only? Without a surrounding "SCIP_CALL", this raises warnings, so this
>> > is not nice in functions that need to return something different than
>> > "SCIP_RETCODE", which is common in C++ code.
>>
>> These are two questions. First, according to the coding rules, it should
>> be SCIPALLOCMEMORY. Since memory functionality is used at many places, I
>> suppose that it just looked ugly, so the lower case was used.
>>
>
> Why not make it a function then? With a good compiler it would be inlined
> anyway, so no overhead whatsoever.
>

Also this might be true, but relying on a compiler and hoping that it will
do all the smart things for us, will not produce fast code in the end.

>
>>
>> Second, SCIP_CALL is used for the SCIP internal error passing. If you do
>> not like it, you do not have to use it (but at the loss of having a
>> backtrace of SCIP functions). See also the SCIP introduction by
>> Cornelius Schwarz (http://scip.zib.de/further.shtml), which provides a
>> macro to use exceptions.
>>
>> Moreover, returning more than one argument is always complicated in
>> C/C++. The price you have to pay for the SCIP error passing is that you
>> need to return values via pointers.
>>
>
> FYI, it's not in C++, just use a std::pair or std::tuple.
>

Returning pairs or tuples doesn't make it easier to read. And as Marc
already explained, the idea was to get an error passing mechanism. It is
still possible to catch the return value of a "SCIP_RETCODE-function".
Also, SCIP_CALL_ABORT can be used for that and then there is no need to
change the return value in your code. And I think, it is very nice that
you will be warned when not catching a return value.

>
>>
>> > 3. With "SCIPgetSol(...)", why can one refer to the solution of a LP
>> via
>> > "NULL"? My intuition says that with "NULL", there is no partial
>> > structure it is referring to, so I would expect to get the current
>> > _global_ solution.
>>
>> Well, you would need two functions otherwise, so it was just easier to
>> use NULL. It also comes in handy at several places in the code.
>>
>> But in principle, two different things get mixed up here: LP solutions
>> (relaxations) and primal solutions.
>>
>
> Can you please clarify? I don't see why two functions would be necessary.
>

You still need to differentiate between the current LP solution and the
best primal solution as you suggested. But therefor, you only have one
special value like NULL.
The LP solution is a local solution return by an LP solver most likely not
integral and probably not fulfilling some global constraints and therefore
not feasible w.r.t. the global optimization problem. Primal solutions are
feasible solutions w.r.t. the global problem.

>
>>
>> > 4. Concerning the C++ interface, it is mildly disturbing to have
>> dynamic
>> > memory management in function calls, i.e. you have to construct
>> objects
>> > with "new Object", without ever calling delete on them. This is
>> > considered bad practice in C++, which prefers the "RAII" idiom.
>> "Whoever
>> > constructs an object,destroys it" should be obeyed. Why was this
>> > violated in SCIP? See your TSP example for reference. While it does
>> not
>> > leak resources, it is not visible/obvious to the user since there is
>> no
>> > "delete".
>>
>> I am not sure whether I understand you correctly. If you want to delete
>> the objects yourself, you can do so: See for example
>> SCIPincludeObjConshdlr() - set deleteobject to TRUE.
>>
>
> Ah, well i should have looked into the documentation for that one. That's
> the reason I dislike boolean values in function calls by the way: The
> purpose is not clear at all, better use enum.
>

Imo, in most cases boolean values are better understandable then enums and
also enums seem to make you wonder whether there are more then two
possible values.

>
>>
>> > 5. Why does "SCIPinfinity" take a "SCIP*" as argument? Shouldn't this
>> be
>> > a compile time constant independent of any object?
>>
>> The infinity value can in principle be changed, so it is not a compile
>>  time value.
>>
>
> What for?

It might be, that you really need such huge values, and this SCIP
parameter is really changed in some projects. DBL_MAX or something similar
cannot be exceeded and also therefore makes computations much harder.
Also, you will lose accuracy and even get wrong results when working with
really big values, meaning it does not really make sense to increase the
infinity value to the maximal possible value.

>
>
>>  > 6. What led to the decision developing in C?
>>
>> First, the predecessor SIP of SCIP was written in C. Second, the C++
>> standard rapidly changed when the SCIP project started (2002), while C
>> is quite stable (there is hardly any problem with SCIP and compilers).
>>  Third, C can be compiled faster.
>>
>
> "C can be compiled faster" I seriously doubt that ;-) Have you tried? I
> just did with scip.c and it turns out it needs 11s with gcc and exactly
> the
> same time with g++. Execution time is identical if the SCIP source code is
> compiled as c++, so no downside from that perspective.
> C++ did not change in 2002, in fact there was no change between 1999 and
> 2011.
>

Additionally to Marcs arguments, I think, C is much more readable then
assembler and also easier to write ;). Imo, C still gives you the best
chance on implementing fast algorithms and code.
Changing the compiler to compile C code, does not falsify or confirm Marcs
statement, which I would also go with.

>> 7. Your code is not compliant with the C99 standard, since you use
>> double underscores as prefix to identifiers (e.g. in your header include
>> guards).

Actually, we try to be near C90, which also resulted from the idea of
being compilable at many different platforms.

Best, Michael