Re: [gsoc] Discuss thresholding API design

From: Mateusz Loskot (mateusz_at_[hidden])
Date: 2019-05-30 08:32:11

On 19-05-29 20:05:57, Stefan Seefeld wrote:
>On 2019-05-29 7:15 a.m., Mateusz Loskot wrote:
>>
>>Miral and I, we have been brainstorming on what would be a clear yet
>>optimal interface for the thresholding, on Gitter and comments at
>>https://github.com/BoostGSoC19/gil-miral/pull/1
>>
>>This made me think the issue of interface design for new algorithms 
>>is more general,
>>not just specific to the thresholding. I think it may be worth to 
>>set some common
>>guidelines, to tune ourselves w.r.t. interface design in the 
>>direction that may be
>>preferred in GIL.
>
>While I think I understand your concern (and mostly agree), I'm not 
>sure it's useful or even possible to provide general guidelines. 
>Designing good APIs is hard, requires domain knowledge and almost 
>always involves judgment calls.

I might have not expressed my intentions well.
Let me spell out summary of my guidelines:

- Keep it simple, not too clever (e.g. cv::threshold falls into too clever)
- Reach functions set over modest functions set with reach arguments.
- Single responsibility principle (e.g. one algorithm = one function)
- Cohesion at function level (do elements inside a function belong together?)
- Open-close principle (.e.g. new functions are good, new arguments to existing
  functions not necessarily)

My intention was to raise our awarness (esp. of our students), tune our
sensitivity to those aspects, especially when one is looking up to existing
libraries and interfaces. Not all turn to be designed well.

>>### Problem Example
>>
>>For instance, OpenCV takes approach of single function to 'kill'em 
>>all', i.e.
>>cv::threshold, that takes multiple parameters where combination of 
>>values and
>>flags is significant
>>https://docs.opencv.org/4.1.0/d7/d1b/group__imgproc__misc.html#gae8a4a146d1ca78c626a53577199e9c57
>>
>>
>>In this approach documentation is crucial to explain what 
>>combination is valid,
>>when `maxval` is used (e.g. with THRESH_BINARY) and when ignored.
>
>Yeah, I agree this is not a good design. :-)
>
>For once, the abstraction seems more harm- than useful. In addition to 
>the special-casing of what parameter combinations are valid, I'd 
>reason that some of the functions are in fact doing something else. 

Too much of abstraction is exactly my concern leading to raising this issue.

>For example, I'm having a hard time understanding the concept behind 
>the "INV" variants. Shouldn't that be a separate operation ? Why is 
>inversion mixed into the thresholding to begin with ? Is that such a 
>common combination ?

That is about inversing meaning of threshold, see the graphics here:
https://docs.opencv.org/4.1.0/d7/d1b/group__imgproc__misc.html#gaa9e58d2860d4afa658ef70a9b1115576

For example:
    px >= threshold ? max : px
versus
    px <= threshold ? max : px
as Miral captured it in
https://github.com/BoostGSoC19/gil-miral/pull/1/files

>Sometimes the rationale to combine multiple "variants" into a single 
>call is that the implementations share a lot of code. In that case it 
>might be better to put a common _impl function into some `detail` 
>namespace and then call that from different frontend functions with 
>suitable parameters.

Spot on! That is what we are trying to learn and follow with Miral.

>I agree with you that for the example you chose, distinct functions 
>will actually make the API more readable.

I'm happy to hear. Thank you for reviewing the proposal.

>(And for the record: I also agree that the mailing list is a better 
>fit for such design discussions than a gitter or other channel)

Yes, I will suggest Miral and Olzhas to use the list and Gitter for
hands-on support when getting stuck with commands at hand :-)

Best regards,
-- 
Mateusz Loskot, http://mateusz.loskot.net
Fingerprint=C081 EA1B 4AFB 7C19 38BA  9C88 928D 7C2A BB2A C1F2