Unexpected return value for corner_foerstner

[tonysyu]

corner_foerstner returns a tuple of error and roundness measures which can be converted into an array suitable for use with corner_peaks. All other corner detectors return an array that can be used directly with corner_peaks. It'd be ideal if corner_foerstner did the same.

One possible deprecation procedure: Create a slightly mangled name (maybe corner_forstner, which appears to be the more common spelling) to make sure no existing code gets broken, then move the logic of the current detector to something like response_forstner. The new function, corner_forstner would add roundness and error thresholds as parameters, which would basically do what the docstring example does. The old function, corner_foerstner, would be deprecated and anyone who wants to use the raw response could use response_forsnter.

[JDWarner]

I like this plan!

[ahojnnes]

Yes, my implementation is definitely inconsistent here.

Although, the letter "ö" in Förstner is German and the usual replacement in ASCII is "oe". I'm not sure if many people will use this function until 0.9 is released... so, I'm in favor of moving the current implementation to response_foerstner and use corner_foerstner for the thresholded variant. What do you think?

[tonysyu]

As for the spelling, I was just going by the number of google search results (which match both "ö" and "o" forms). My only reason for suggesting the alternate spelling was to avoid breaking code.

I agree that the function is unlikely to see heavy usage between now and 0.9, but API breakage is usually pretty bad. In this case, a user should see an error pretty early on (since a tuple of arrays is really different than a single array), so at least it's easy to catch.

I could go either way. @stefanv: Thoughts?

[ahojnnes]

Side-note: I don't have a strong opinion regarding "oe" vs. "o"... just wanted to give some insight from a native German speaker ;-)

[stefanv]

Let's just bite the bullet on this one and do the API fix with breakage. We should obviously avoid doing this wherever possible, but I don't see a clean way out of this one. Just document it in the API breakages file.

[tonysyu]

+1

[tonysyu]

We should try to have some sort of consensus for breaking API. How do people feel about it? @JDWarner, @ahojnnes, @emmanuelle?

[JDWarner]

IMO break it, the proposed (harmonized) method is superior. The sooner the better, too; it's only going to get more disruptive.

[ahojnnes]

Add threshold and roundness thresholds to the current function... People will definitely notice the change and can easily fix it.

[ahojnnes]

Hm, I just wanted to change my implementation and got to think about it again. There is no one correct way to apply the thresholds to w and q. I changed my mind: I'd keep the inconsistency and leave it as is...

[tonysyu]

I can see your point. But the naming suggests more similarity to the other corner detectors than this provides. Maybe some sort of alternate name? I'm not sure what that would be without making it unnecessarily long... Actually, I'd be happy with a note just under the header line of the docstring clearly stating that the result is drastically different than the other corner detectors.

[stefanv]

@ahojnnes @tonysyu Do you have any further opinions about this? It sounds as if you agreed that corner_foerster should just be different?

[tonysyu]

Yeah, it's probably fine to close this issue. I still think it would be nice to rename the function so it's a bit clearer that it returns something very different from the other corner detectors, but I don't have a good suggestion for a rename.