@amyeroberts I have some questions about the implementation and AutoModel classes.

First of all, I try to follow as much as possible the patterns I see in other model implementations (resnet or convnextv2 for example), but unlike these models, SuperPoint really only have one function or "mode" here, just to output the keypoints, their scores and descriptors. This is why I only implemented SuperPoint as a SuperPointModelForInterestPointDescription, so there is no SuperPointModel anymore, does that seem ok ?

Then I added this SuperPointModelForInterestPointDescription class in a new mapping dictionary in the modeling_auto file and added the appropriate AutoModel class for this. But is this kind of changes usually the output of an automated script for model registration or adding it by hand is appropriate ?

Finally,

In that PR, we can also add a mapping AutoModelForInterestPointDescription, which we define as taking two images and returning interest keypoints and their descriptions.

Apart from adding the AutoModelForInterestPointDescription, I couldn't find how to define such inputs and outputs, is it a new pipeline I should define or something else ?

Hi @sbucaille,

This is a bit of a special case. For other models which only perform a single task, what we normally do is just have XxxModel. I'd suggest doing this. We can still add AutoModelForInterestPointDescription and have SuperPointModel loaded by it

@sbucaille From next week, I'll be off for a few weeks. If you have vision-specific questions, please ping @rafaelpadilla; for implementation questions @ArthurZucker.

Hi @ArthurZucker, I added the SuperPointImageProcessor as part of the code because SuperPoint requires a grayscale image as input. But when I added the tests I have the test_call_pil which fails and give me a very weird when it reaches these lines

tests/models/superpoint/test_image_processing_superpoint.py:43: in prepare_image_inputs
    return prepare_image_inputs(
tests/test_image_processing_common.py:64: in prepare_image_inputs
    image_inputs = [Image.fromarray(np.moveaxis(image, 0, -1)) for image in image_inputs]
tests/test_image_processing_common.py:64: in <listcomp>
    image_inputs = [Image.fromarray(np.moveaxis(image, 0, -1)) for image in image_inputs]

with the following error :

            except KeyError as e:
                msg = "Cannot handle this data type: %s, %s" % typekey
>               raise TypeError(msg) from e
E               TypeError: Cannot handle this data type: (1, 1, 1), |u1

Not sure what causes the problem as I tried to compare with tests made with the ConvNextImageProcessor which does not raise any error.

Anyway, I continue on the implementation, let me know if I'm missing anything. I'll write the documentation for all the code I've previously pushed.

Hi @ArthurZucker, I added the SuperPointImageProcessor as part of the code because SuperPoint requires a grayscale image as input. But when I added the tests I have the test_call_pil which fails and give me a very weird when it reaches these lines

tests/models/superpoint/test_image_processing_superpoint.py:43: in prepare_image_inputs
    return prepare_image_inputs(
tests/test_image_processing_common.py:64: in prepare_image_inputs
    image_inputs = [Image.fromarray(np.moveaxis(image, 0, -1)) for image in image_inputs]
tests/test_image_processing_common.py:64: in <listcomp>
    image_inputs = [Image.fromarray(np.moveaxis(image, 0, -1)) for image in image_inputs]

with the following error :

            except KeyError as e:
                msg = "Cannot handle this data type: %s, %s" % typekey
>               raise TypeError(msg) from e
E               TypeError: Cannot handle this data type: (1, 1, 1), |u1

Not sure what causes the problem as I tried to compare with tests made with the ConvNextImageProcessor which does not raise any error.

Anyway, I continue on the implementation, let me know if I'm missing anything. I'll write the documentation for all the code I've previously pushed.

Hi @sbucaille, 🙂

A quick help with that issue:

I see that your processing converts all images to grayscale (here) and tests are failing here.

The root cause is that the convert_to_grayscale function returns a 1-channel image (luminance) as here. So, when it is later converted to a numpy array, it will turn to be a 1-channel image, making the test fail.

This has been discussed in PR #25767 and is not fully solved yet.

A quick solution for this issue in your code may be possible. See that a 1 single channel image is definetely grayscale, but if the 3 channels in an RGB image are equal (R==G and G==B), the image is also noted as grayscale. So, if you replicate the channels of your 1-channel grayscale image as in here, this issue can be solved. However, SuperPoint would need to be adapted for that -> you would only need to consider one of the RGB channels, as they are equal.

Hi @rafaelpadilla and @ArthurZucker ,
Thanks @rafaelpadilla for the heads up, I adapted SuperPointModel and SuperPointImageProcessor to cover this issue. SuperPointImageProcessor now generates a 3-channel grayscaled image from a given image as input and SuperPointModel extracts one of the channels to perform the forward method. Although it may be necessary to change that in the future when 1-channel images will be supported (if it is planned to).
I added docs, as well as remaining integration tests for the AutoModelForInterestPointDescription.
I think the implementation is complete.

@ArthurZucker, please let me know what I'm missing in the implementation ! 🙂
Although I have some questions :

• What should I do with the SuperPointModel and SuperPointModelForInterestPointDescription as both are basically the same, should I only keep the latter one ?
• Regarding docs, there is a mention of expected_output is @add_code_sample_docstrings. I decided not to provide such information since output is dynamic, depending on the number of keypoints found. Should I keep it like that or there is a way to provide a "dynamic shape" to this function ?
• Regarding tests, I have the test_model_is_small failing, what should I do about it ? And is test_retain_grad_hidden_states_attentions related to models that can be trained ? If so we should probably skip it since SuperPoint can't be trained, also it does not have attentions.

Hi,
When adding the docs to the code on this Saturday, I started thinking, maybe late, about the licence of SuperPoint and got an answer from an original contributor, Paul-Edouard Sarlin.
It turns out it can't be used for commercial use. I am not very familiar with legal stuff like these, but does it compromise this PR ? Or, from the HuggingFace perspective, adding the licence as in the original repo is sufficient ? I added it to the model card anyway.

Hi, When adding the docs to the code on this Saturday, I started thinking, maybe late, about the licence of SuperPoint and got an answer from an original contributor, Paul-Edouard Sarlin. It turns out it can't be used for commercial use. I am not very familiar with legal stuff like these, but does it compromise this PR ? Or, from the HuggingFace perspective, adding the licence as in the original repo is sufficient ? I added it to the model card anyway.

Hi @sbucaille,

It seems that the original code is under MIT license. If it is the case, you just need to add the MIT license on the top of the files, as done in graphormer and IDEFICS.

The checkpoints seem to be under a non-commercial customized license. So, as you have already added the License here, you just need to set inference: false in the card as done in owlvit-large-patch14 and musicgen-large.

@sbucaille

Sorry about this, but I am afraid there is nothing we can help on this situation (at least not from me).
One tip (for future PRs): don't use git merge, use git rebase to keep the commits linear. It will avoid such undesired situation.

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.