Keyword argument names incorrectly cased

[davydog187]

Hello @cocoa-xu, thanks for this amazing project 🥳!

I started porting a python script we use internally to Evision, and got stuck when working with Evision.FlannBasedMatcher.flannBasedMatcher/1. The docs describe the options as:

Keyword Arguments

indexParams: flann::IndexParams.
searchParams: flann::SearchParams.

It turns out that these should be passed as index_params and search_params, rather than the camelCased name as described. As far as I can tell, something around here needs some adjustment.

Additionally, it doesn't seem that the available flann::IndexParams and flann::SearchParams are documented anywhere. Is this an oversight?

[cocoa-xu]

Hi @davydog187, thanks for reporting this issue! I checked the generated C++ binding code and it seems that these should be the camelCased ones.

As for the second questions, this function in Evision expects a map (basically the same as a dictionary in Python) and then we have similar code as Python:

In Python

Code from https://docs.opencv.org/4.9.0/dc/dc3/tutorial_py_matcher.html

import numpy as np
import cv2 as cv
import matplotlib.pyplot as plt

img1 = cv.imread('box.png',cv.IMREAD_GRAYSCALE) # queryImage
img2 = cv.imread('box_in_scene.png',cv.IMREAD_GRAYSCALE) # trainImage

# Initiate SIFT detector
sift = cv.SIFT_create()

# find the keypoints and descriptors with SIFT
kp1, des1 = sift.detectAndCompute(img1,None)
kp2, des2 = sift.detectAndCompute(img2,None)

# FLANN parameters
FLANN_INDEX_KDTREE = 1
index_params = dict(algorithm = FLANN_INDEX_KDTREE, trees = 5)
search_params = dict(checks=50) # or pass empty dictionary
flann = cv.FlannBasedMatcher(index_params,search_params)

Evision

img1 = Evision.imread("box.png", flags: Evision.Constant.cv_IMREAD_GRAYSCALE()) # queryImage
img2 = Evision.imread("box_in_scene.png", flags: Evision.Constant.cv_IMREAD_GRAYSCALE()) # trainImage

# Initiate SIFT detector
sift = Evision.SIFT.create()

# find the keypoints and descriptors with SIFT
{kp1, des1} = Evision.SIFT.detectAndCompute(sift, img1, Evision.Mat.empty())
{kp2, des2} = Evision.SIFT.detectAndCompute(sift, img2, Evision.Mat.empty())

# FLANN parameters
flann_index_kdtree = 1
index_params = %{"algorithm" => flann_index_kdtree, "trees" => 5}
search_params = %{"checks" => 50} # or pass empty dictionary
flann = Evision.FlannBasedMatcher.flannBasedMatcher(indexParams: index_params, searchParams: search_params)

[davydog187]

Ah, thank you @cocoa-xu this is very helpful! Initially I thought this was still not correct, but it appears something else was wrong with my code.

As a suggestion for improvement, could Evision generate a call to Keyword.validate! to help users if they pass the wrong option? Until we get a type system this could really help with the developer experience

[davydog187]

Also I noticed in your example you use 1 for flann_index_kdtree, while Evision.Constant.cv_KDTREE() is 2. Was that intended or are these different constants?

[cocoa-xu]

As a suggestion for improvement, could Evision generate a call to Keyword.validate! to help users if they pass the wrong option? Until we get a type system this could really help with the developer experience

This definitely could help but I'm not sure how much performance it would hurt... Hopefully it should be a very mild one.

But even in that case, it won't be able to check if these options are the ones for the intended overloaded function (i.e., some functions in OpenCV have multiple overloads and can take different set of options).

Also I noticed in your example you use 1 for flann_index_kdtree, while Evision.Constant.cv_KDTREE() is 2. Was that intended or are these different constants?

Since the code generation part is adapted from the same code that used to generate opencv-python, current version has basically the same limitation as in Python, i.e., some enumerate types are not included in the final code, (that's also the reason they're using FLANN_INDEX_KDTREE = 1 in the Python example).

The Evision.Constant.cv_KDTREE() thing you mentioned is actually the value of cv::ml::KNearest::KDTREE.

This could be changed though, I'll see what I can do with it in the coming week or so. But it will surely be a breaking change.

[cocoa-xu]

This definitely could help ... take different set of options)

Besides that, these keyword arguments in the inline docs were generated using the same metainfo as the generated binding code, so they can't be wrong. Maybe I can add them in the function specs so at least the language server can potentially suggest the correct ones (though you still have to check which options are used by the intended overloaded function).

[davydog187]

Since it's usually a small (<32) number of options, performance overhead is probably negligible.

[davydog187]

But even in that case, it won't be able to check if these options are the ones for the intended overloaded function (i.e., some functions in OpenCV have multiple overloads and can take different set of options).

If that's the case, even the union of all overloaded options would be a huge improvement!

[cocoa-xu]

Hi @davydog187, v0.1.39 is shipped with the requested feature!

[davydog187]

You are amazing @cocoa-xu 🥳 thank you!