SAC Models Refactoring: isModelValid()

[taketwo]

The isModelValid() function in every SAC model class begins by verifying that the number of coefficients in the passed model is as expected. This pull request extracts this check into the base implementation of this function.

[jspricke]

Hi Sergey, sorry for being picky again, can you elaborate a little why we want this?

[taketwo]

As stated in the title, in this pull I did some refactoring. It does not change functionality in any principal way, but rather rearranges internal stuff (by removing copy/pasted code), the purpose being improved maintainability and readability of the code.

In particular, the function getModelSize() that this pull request adds provides the expected number of coefficients for each model in a uniform way. It is implemented in exactly the same way as already existing getSampleSize(), which provides the minimum sample size for each model type.

[jspricke]

Yeah, I got what you did, but why? I would rather go the opposite way and move getSampleSize() into each model. At the moment you need to remember to adopt getSampleSize() when adding a new model. This is not intuitive and object oriented like. Moving everything into the respective models, would make them independent. What do you think?

[taketwo]

Ah, now I get your point. Yes, this externalized constants seemed strange to me as well. There might be some reason why this is implemented in this particular way. I do not know it, but I simply assumed that there is a reason and changed the other part to match the style.

Personally, I would I implement it the way you suggested, i.e. with virtual getSampleSize()/getModelSize() that each model overrides with it's own number. If you agree, I can go ahead and get rid of those size and model lookup tables.

[jspricke]

Would be great :).

[taketwo]

I wonder what would be better:

• virtual getSampleSize(), each model overrides and returns its number

or

• inline getSampleSize() that returns protected member sample_size_, which is set at construction time in each model

The first one is a bit more straightforward, but the second one is presumably faster, because no virtual calls (and getSampleSize() is called really often).

[jspricke]

Shouldn't it be enough to make it a public const and access it directly?

[taketwo]

Won't work, because if we make something public const in the base class, then it should be initialized in the initializer list of the base class constructor. Deriving classes will not be able to modify it later because it is const.

[jspricke]

What about something like: http://stackoverflow.com/questions/9999952/set-const-member-value-from-subclass

[taketwo]

This could work!

[stefanbuettner]

Hey there,

hope you don't mind me throwing in some thoughts of mine about this topic:
Is exposing variables to the public API not a bad idea in general? Consider a scenario where the variable's value depends on other values and is required to be computed on the fly. E.g. a model for general surfaces of revolution bounded by polynomials of degree n. Allowing the degree to change after object creation would change the number of model coefficients and the number of samples required. This would be impossible to realize with that approach. So if you want to prepare for scenarios like this, I think virtual methods should be used. If you don't want to allow such scenarios, I would prefer taketwo's second suggestion because it's still fast and the way the value is determined can still change in the future (even allowing on-the-fly computations) without changing the public API.

Making the getSampleSize method pure virtual, as I did in my suggestion has the advantage, that it needs to be implemented in direct subclasses, so there is no way of forgetting it for those. However in classes further down the hierarchy (like parallel plane), which could potentially have different sample/coefficient sizes, one had to think of it again. Furthermore declaring it pure virtual could potentionally break existing client subclasses which do not implement getSampleSize yet, for obvious reasons. So this was probably a bad idea in my suggestion.

Cheers,
Stefan

[taketwo]

Hi Stefan, thanks for the inputs. I think exposing variables to the public API is a no-go, we should have a getter function. As I proposed before

inline getSampleSize() that returns protected member sample_size_, which is set at construction time in each model

We can mark it const, but this will rule out your idea of selecting model type at runtime. So I don't see any benefit in doing so.

Making the getSampleSize method pure virtual, as I did in my suggestion has the advantage, that it needs to be implemented in direct subclasses, so there is no way of forgetting it for those.

This can be addressed by adding compulsory sample_size parameter to each constructor of the base class.

What do you think?

[stefanbuettner]

Sounds good. It should solve the problem of forcing the programmer to specify sample and model sizes like the pure virtual methods but presumably faster. But it also does not solve it for second level subclasses (which I think cannot be sovled and perhaps is also not worth thinking about, because someone implementing a new model will think about and check those values anyway.).

But I just thought about the Semantic Versioning System. Is this applied to PCL? Cause then adding a compulsory parameter to the constructors of SampleConsensusModel would be a backwards incompatible change, wouldn't it? And if we assume that users of the SampleConsensusModel class, who will be implementing a new SAC models, will check these parameters anyway, there would be no need to make it compulsory or pure virtual.

So for the 1.8.0 version I'd just introduce the sample_size parameter and add a compulsory parameter to the constructors for version 2.0.0.

[jspricke]

+1 for the last proposal by @stefanbuettner. Can both of you coordinate who will send a pull request? Thanks for the discussion!

[taketwo]

I'll take care of it.