Add SuperGlue model
Author: sbucaille
Pull request: yes
Labels: name: New model

name: Vision

name: run-slow

# What does this PR do?

<!--
Congratulations! You've made it this far! You're not quite done yet though.

Once merged, your PR is going to appear in the release notes with the title you set, so make sure it's a great title that fully reflects the extent of your awesome contribution.

Then, please replace this with a description of the change and which issue is fixed (if applicable). Please also include relevant motivation and context. List any dependencies (if any) that are required for this change.

Once you're done, someone will review your PR shortly (see the section "Who can review?" below to tag some potential reviewers). They may suggest changes to make the code even better. If no one reviewed your PR after a week has passed, don't hesitate to post a new comment @-mentioning the same persons---sometimes notifications get lost.
-->

<!-- Remove if not applicable -->

Fixes #25489 
This PR is the next step after implementing [SuperPoint](https://github.com/huggingface/transformers/pull/28966) to implement image matching through keypoint matching.

**Colab notebook with inference example:**
https://colab.research.google.com/drive/1NhwofZFzy7IMN4irN-jC-9LZy7dx_GZ2?usp=sharing

## Who can review?

@amyeroberts 


Comments:
@amyeroberts Alright, the PR is opened, even though the branch is still a bit of a mess but the code contained in the `superglue` folder is more or less what it would look like in the end I guess.
Although, I have several questions / problems :
1. I tried to follow the LlaVA example, which takes an arbitrary vision config for the vision task and implemented the same way in SuperGlue, but when I instantiate my `keypoint_detector` using `AutoModel.from_config`, SuperPoint weights are not loaded. So I need to force the instantiation using `AutoModelForKeypointDetection` with the `keypoint_detection_config.name_or_path`, is this the intended use ? (line 390 of `modeling_superglue.py`)
2. What SuperGlue models should be implemented ? A `SuperGlueForImageMatching` containing an `AutoModelForKeypointDetection` with inputs in forward as regular `pixel_values` ? A `SuperGlueForKeypointMatching`  without keypoint detector included but with inputs of the forward as the keypoints, scores and descriptors of 2 images ?
3. In the example of `SuperGlueForImageMatching` , how should we treat pixel_values ? For now I raise an error of the batch size if not modulo 2 (the remaining code does not cover batching yet)
4. About the SuperPoint PR, I couldn't see it in the release note, is it a mistake ? 😬 

@ydshieh Hey, got a weird bug when I try to run `make fix-copies` in order to ensure SuperGlue is imported in all the necessary files and it crashes there : 
````
❯ make fix-copies
python utils/check_copies.py --fix_and_overwrite
Traceback (most recent call last):
  File "/home/steven/transformers_fork/transformers/utils/check_copies.py", line 315, in split_code_into_blocks
    ).groups()[0]
AttributeError: 'NoneType' object has no attribute 'groups'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/steven/transformers_fork/transformers/utils/check_copies.py", line 1218, in <module>
    check_copies(args.fix_and_overwrite, args.file)
  File "/home/steven/transformers_fork/transformers/utils/check_copies.py", line 852, in check_copies
    new_diffs = is_copy_consistent(filename, overwrite, buffer)
  File "/home/steven/transformers_fork/transformers/utils/check_copies.py", line 675, in is_copy_consistent
    target_lines, theoretical_code, theoretical_code_splits = find_code_and_splits(
  File "/home/steven/transformers_fork/transformers/utils/check_copies.py", line 529, in find_code_and_splits
    code_splits = split_code_into_blocks(
  File "/home/steven/transformers_fork/transformers/utils/check_copies.py", line 319, in split_code_into_blocks
    raise ValueError(
ValueError: Tried to split a class or function. It did not work. Error comes from line -1: 
```
        help="Name of the model you'd like to convert.",
    )
    parser.add_argument(
        "--pytorch_dump_folder_path", default=None, type=str, help="Path to the output PyTorch model directory."
    )
    parser.add_argument(
        "--push_to_hub", action="store_true", help="Whether or not to push the converted model to the 🤗 hub."
    )

    args = parser.parse_args()
    convert_dinov2_checkpoint(args.model_name, args.pytorch_dump_folder_path, args.push_to_hub)
```

make: *** [Makefile:84: fix-copies] Error 1

````

This seems to come from `dinov2/convert_dinov2_to_hf.py` which makes no sense. Any idea ?

@sbucaille Apologies for the delay in my response. I've answered your initial questions below - let me know if anything isn't clear

> I tried to follow the LlaVA example, which takes an arbitrary vision config for the vision task and implemented the same way in SuperGlue, but when I instantiate my keypoint_detector using AutoModel.from_config, SuperPoint weights are not loaded. So I need to force the instantiation using AutoModelForKeypointDetection with the keypoint_detection_config.name_or_path, is this the intended use ? (line 390 of modeling_superglue.py)

This shouldn't be necessary. Doing: 

```py
self.keypoint_detector = AutoModelForKeypointDetection.from_config(config.keypoint_detector_config)
```

is the correct way to go. If the weights are all newly initialized, then first thing I'd suspect is that the checkpoint being used doesn't contain the SuperPoint weights. 

> What SuperGlue models should be implemented ? A SuperGlueForImageMatching containing an AutoModelForKeypointDetection with inputs in forward as regular pixel_values ? A SuperGlueForKeypointMatching without keypoint detector included but with inputs of the forward as the keypoints, scores and descriptors of 2 images ?

SuperGlueForImageMatching taking `pixel_values`. I'll think about the name to see if there's something better we can use. "ImageMatching" is a bit ambiguous. 

> In the example of SuperGlueForImageMatching , how should we treat pixel_values ? For now I raise an error of the batch size if not modulo 2 (the remaining code does not cover batching yet)

In this case, are the images to be pair interleaved? e.g. the sequence goes [image_1_a, image_1_b, image_2_a, image_2_b, ...]

I think I would do something more similar to sentence similarity for text models: 
* The image processor take a pair of images or a list of pair of images
* The images are processed and returned as a tensor of shape (B, 2, C, H, W)

> About the SuperPoint PR, I couldn't see it in the release note, is it a mistake ? 😬

This is just an oversight, sorry, as the model as added last minute as a patch after the branch was already cut for release. I'll update the notes. 

@ydshieh Nevermind, found the problem, it came from a left ouf `# Copied from transformers.models.dinov2.convert_dinov2_to_hf` above a function, sorry for the inconvenience.

@amyeroberts Everything is clear, I fixed the embedded `keypoint_detector` state dict loading in the `convert_superglue_to_hf.py` script. So we agree that saving a `SuperGlueForImageMatching` weights in a new Hub repo necessarily means we also save the `keypoint_detector` weights with it right ? Is this how it is implemented in LlaVa ?

I'll implement the new `SuperGlueImageProcessor` later, but I was trying out inferences, I noticed a difference in terms of result with the way we read an image. 
In SuperPoint it was not that noticeable, but for example, with the two images I've added in this PR, using the `SuperPointImageProcessor` (which uses `PIL.Image.open()`) ends up having SuperGlue matching 144 points where using the original authors code for reading (see below in the post), ends up having SuperGlue matching 175 points (I reach this number whether I run their original code or when I added it in the `convert_superglue_to_hf.py` script). Since their implementation uses OpenCV instead of PIL, should we change the `SuperPointImageProcessor` ? What is the convention in transformers for image reading libraries ? This difference has a negative snowball effect on the performance, although I haven't tried on many examples.

The original read_image function from authors : 
```python
def frame2tensor(frame, device):
    return torch.from_numpy(frame/255.).float()[None, None].to(device)


def read_image(path, device, resize, rotation, resize_float):
    image = cv2.imread(str(path), cv2.IMREAD_GRAYSCALE)
    if image is None:
        return None, None, None
    w, h = image.shape[1], image.shape[0]
    w_new, h_new = process_resize(w, h, resize)
    scales = (float(w) / float(w_new), float(h) / float(h_new))

    if resize_float:
        image = cv2.resize(image.astype('float32'), (w_new, h_new))
    else:
        image = cv2.resize(image, (w_new, h_new)).astype('float32')

    if rotation != 0:
        image = np.rot90(image, k=rotation)
        if rotation % 2:
            scales = scales[::-1]

    inp = frame2tensor(image, device)
    return image, inp, scales
```


> So we agree that saving a SuperGlueForImageMatching weights in a new Hub repo necessarily means we also save the keypoint_detector weights with it right ? Is this how it is implemented in LlaVa ?

Yep, that's how it's done in Llava and other composite models. You can check this by inspecting the safetensor weight names on the hub for a checkpoint: https://huggingface.co/llava-hf/llava-v1.6-mistral-7b-hf/tree/main (Click on this symbol: <img width="44" alt="image" src="https://github.com/huggingface/transformers/assets/22614925/db320671-7c36-4292-8151-2df97cbf85aa">)

> In SuperPoint it was not that noticeable, but for example, with the two images I've added in this PR, using the SuperPointImageProcessor (which uses PIL.Image.open())

None of our image processors should be using `PIL.Image.open`. They should take already opened images. I can't see [where it's used in the image processor](https://github.com/huggingface/transformers/blob/6cdbd73e01a9719bfaec07d91fd108e8d932bbbb/src/transformers/models/superpoint/image_processing_superpoint.py#L4)? 

The image processors accept `PIL.Image.Image`, `torch.tensor`, `jax.array`, `tf.tensor` or `np.array`, but it's not responsible for loading / opening images. If for testing the port you want to use cv and then convert the images to one of these formats, that should be OK. 

It would be interesting to know what the difference is between the two images after loading from PIL vs cv2 

 @amyeroberts Indeed there is no such thing of reading an image in the image processors, I was mistaken. 
> The image processors accept `PIL.Image.Image`, `torch.tensor`, `jax.array`, `tf.tensor` or `np.array`, but it's not responsible for loading / opening images. If for testing the port you want to use cv and then convert the images to one of these formats, that should be OK.
> It would be interesting to know what the difference is between the two images after loading from PIL vs cv2

About that, I've modified the `convert_superglue_to_hf.py` script and added multiple ways of reading the image. After some experiment, it looks like reading the image with `PIL.Image.open()`, `cv2.imread(..., cv2.IMREAD_GRAYSCALE)` or `cv2.imread(..., cv2.IMREAD_COLOR)` (with conversion from BGR to RGB) results in the same number of matching. I haven't looked at the images themselves but the only difference now resides in the way images are resized, in the original code it is done with cv2 whereas in transformers it is done with PIL by default. As I said earlier, I haven't tried on many images so maybe this pair of images is indeed impacted in terms of performance, maybe in other cases it won't. How strict is the policy regarding the performance of models being replicated when implemented in transformers ?

On additional notes : 
- I've added the SuperGlueImageProcessor. For obvious reasons, I made this object instantiate a SuperPointImageProcessor as the processing in terms of grayscale is exactly the same. SuperGlueImageProcessor will only make sure there is a pair number of images and pair them interleaved as you suggested. I was also thinking about a "pairing mode" where the user could give an arbitrary number of images and the processor would group them based on the mode. For example, with combinations, having [im0, im1, im2] would result in [(im0, im1), (im0, im2), (im1, im2)] ?
- I've implemented batching in SuperGlue similarly to SuperPoint with a masked output.
- How should `hidden_states` be handled in SuperGlue ? Should it be SuperPoint `hidden_states` or something referring the `keypoint_encoder` or the `gnn` ?
- Also, as a suggestion for the naming, ImageKeypointMatching could be better than just ImageMatching ? And maybe should we rename KeypointDetection references to ImageKeypointDetection ?

@amyeroberts Me again, I had time to write some tests, but I ended up with a very verbose implementation of the forward method. The reason is because in order to cover all tests about hidden states, we need to output 2 variables (`last_hidden_state`, `hidden_states`), but these variables can't be stacked easily as SuperGlue states are sparse tensors and their last dimension depends on the number of keypoints, so we need to store these states for each image, hence we have 4 variables throughout the method (`last_hidden_state_0`, `last_hidden_state_1`, `hidden_states_0`, `hidden_states_1`). In the end, we have 2 variables but these need to be padded tensors like `matches` and `matching_scores`.
This is just the beginning as SuperGlue also includes attention, so these tensors will also need to be taken care of. But considering the current state of the forward method, I preferred to keep it like that and get your opinion on this.

On different tests I have issues :
 - Every tests related to torchscript fail because it is allocating too much memory in my GPU when reaching einsum instructions, is there known issues of einsum with these torchscript tests ?
```
>           return _VF.einsum(equation, operands)  # type: ignore[attr-defined]
E           torch.cuda.OutOfMemoryError: CUDA out of memory. Tried to allocate 82.88 GiB. GPU 0 has a total capacty of 15.99 GiB of which 12.28 GiB is free. Including non-PyTorch memory, this process has 17179869184.00 GiB memory in use. Of the allocated memory 1.09 GiB is allocated by PyTorch, and 85.38 MiB is reserved by PyTorch but unallocated. If reserved but unallocated memory is large try setting max_split_size_mb to avoid fragmentation.  See documentation for Memory Management and PYTORCH_CUDA_ALLOC_CONF
```
- The last test not passing is about initialization, where weight values are not getting the proper value, but the following statement is a bit odd :
```
for name, param in model.named_parameters():
                if param.requires_grad:
>                   self.assertIn(
                        ((param.data.mean() * 1e9).round() / 1e9).item(),
                        [0.0, 1.0],
                        msg=f"Parameter {name} of model {model_class} seems not properly initialized",
                    )
E                   AssertionError: 0.06238596513867378 not found in [0.0, 1.0] : Parameter keypoint_encoder.encoder.layers.0.weight of model <class 'transformers.models.superglue.modeling_superglue.SuperGlueForImageMatching'> seems not properly initialized
```

Last test to write is the one related to the pretrained model and checking whether the model outputs the proper values.
Also some docs missing here and there.

_EDIT : problem fixed_ 

Hey @amyeroberts 
Following up on the PR, I adapted the code to the recent changes of the main branch and noticed several new tests that are not passing on my side (`test_save_load_low_cpu...` ones) with the following error : 
```
        for p1, p2 in zip(model_low_usage.parameters(), model_non_low_usage.parameters()):
>           self.assertEquals(p1.data.ne(p2.data).sum(), 0)
E           RuntimeError: Expected all tensors to be on the same device, but found at least two devices, cuda:0 and cpu!

tests/test_modeling_common.py:504: RuntimeError
```
Is it a known problem from the main branch (I tried with the superpoint tests and ended with the same result) or something on my part ?

_EDIT : problem fixed_ 

@amyeroberts Hey, you can disregard the last two posts I made as I found the origin of the problems.
Although I have a question regarding returning hidden_states and attentions. In SuperPoint, hidden_states are returned up to the point before we have sparse tensors with an arbitrary number of keypoints detected. In SuperGlue, everything computed after the keypoint detector has the same arbitrary dimension depending on the number of keypoints detected. I worked on [this commit](https://github.com/huggingface/transformers/pull/29886/commits/5b3b669c927fce36e2ee7023c07561fe598f606d) for several hours before realizing it 😅. And the tests actually pass since we make sure to have a fixed number of keypoints returned, so all hidden_states and attentions are properly concatenated together, but adding `output_hidden_states=True` to an inference with different number of keypoints per image would crash. But adapting this code for arbitrary number of keypoints will lead to having a lot of lines of code dedicated for creating a mask tensor and filling it with the tensors (which represent already a big portion of the code just for the matches and matching scores).
So, now a question worth asking is : is it relevant to returns such states from SuperGlue ? I would say it makes sense of the model would act just like a backbone but it is far from it in my opinion.
If yes, then I can always implement it, no worries. If no then I'll revert the commit and we can consider the model as fully implemented and we could start the review process and make a final pass on it.

@sbucaille Apologies for the delay in my response on this PR. 

If I've understood correctly, you're asking about whether we should return the hidden states from the SuperPoint model when calling SuperGlue, is that right? 

If so, I'd say no, we don't need to return them and I agree it can act as if it were a backbone. As, technically, any keypoint detection model can feed into super glue, we can't make the assumption about all of them having the same kind of hidden states. 

@amyeroberts Hey ! No worries at all !!
Part of the question was indeed whether SuperGlue should return SuperPoint hidden_states yes. But the other part of the question regarded SuperGlue's hidden states itself, should it return hidden states and/or attention as it involves some MLP's and attention layers ? I assumed no and reverted my changes and SuperGlue does not return anything other than the matches, matching_scores and keypoints along with the mask.
Other previously points need to be addressed : 
- Regarding the `SuperGlueImageProcessor`, I currently copied the behavior from the `SuperPointImageProcessor`, should that rather be instantiated in the `SuperGlueImageProcessor` ?
- Still on the `SuperGlueImageProcessor`, I currently batched images as you suggested : taking a list of images `[image0, image1, ..., image{n-1}, image{n}]` into `[[image0, image1], ..., [image{n-1}, image{n}]]` as a `[B, 2, C, H, W]` tensor, BUT if `n` is odd then the last image is doubled so we still have a pair of images and code is not crashing.
-  For the concern you had about the naming for the model and output with `ImageMatching`, I suggest `ImageKeypointMatching`. And maybe we could rename KeypointDetection references to ImageKeypointDetection ?

All tests passed, should we move onto a first review ?

@sbucaille Ah, OK, I see. Regarding returning hidden_states and attentions, yes, the model should return these. It's fine if the dimension here varies depending on the number of points detected, as long as not all of the dimensions vary (if that makes sense?). That is, for each "block" of the model, there should be an associated hidden_states and attentions tensor which is returned if `output_hidden_states` and `output_attentions` are True. 

For the other points: 
* Regarding the SuperGlueImageProcessor, I currently copied the behavior from the SuperPointImageProcessor, should that rather be instantiated in the SuperGlueImageProcessor ?

Could you clarify this a bit? Specifically instantiation of what and where?

* Still on the SuperGlueImageProcessor, I currently batched images as you suggested : taking a list of images [image0, image1, ..., image{n-1}, image{n}] into [[image0, image1], ..., [image{n-1}, image{n}]] as a [B, 2, C, H, W] tensor, BUT if n is odd then the last image is doubled so we still have a pair of images and code is not crashing.

Is there any valid input to SuperGlue which involves one image? If not, then we can raise an exception when the image processor is called if the number of images isn't even

* For the concern you had about the naming for the model and output with ImageMatching, I suggest ImageKeypointMatching. And maybe we could rename KeypointDetection references to ImageKeypointDetection ?

`ImageKeypointMatching` sounds good to me! Or just `KeypointMatching`

We could, although renaming models should be done lightly as this would be a breaking change. I don't think the `Image` prefix adds much. 

Hey @amyeroberts,


> @sbucaille Ah, OK, I see. Regarding returning hidden_states and attentions, yes, the model should return these. It's fine if the dimension here varies depending on the number of points detected, as long as not all of the dimensions vary (if that makes sense?). That is, for each "block" of the model, there should be an associated hidden_states and attentions tensor which is returned if `output_hidden_states` and `output_attentions` are True.

Alright I made the changes to output the hidden states and attentions, in hidden states only the last dimension is "masked" and equals the highest number of keypoints in all the images, same for attentions for the last and 2nd to last.

> * Regarding the SuperGlueImageProcessor, I currently copied the behavior from the SuperPointImageProcessor, should that rather be instantiated in the SuperGlueImageProcessor ?
> 
> Could you clarify this a bit? Specifically instantiation of what and where?

Since SuperGlue uses SuperPoint as its keypoint detector, the `SuperGlueImageProcessor` needs to process images such that SuperPoint can process them, so for now I copied the part where `SuperPointImageProcessor` turns the images into grey scaled tensors, and added the logic of batching images into pairs after that, my question is should `SuperGlueImageProcessor` use the `SuperPointImageProcessor` as a first "internal" processor so that images complies with SuperPoint requirements and then add the image pair batching logic after or should I keep it like that ? (you can find the code [here](https://github.com/huggingface/transformers/pull/29886/files#diff-de55754b00d62b5234a4e2de9ad9b24b75d4cc16fee3ca7458d7ef05c574e9b4:~:text=class%20SuperGlueImageProcessor(BaseImageProcessor)%3A))

> * Still on the SuperGlueImageProcessor, I currently batched images as you suggested : taking a list of images [image0, image1, ..., image{n-1}, image{n}] into [[image0, image1], ..., [image{n-1}, image{n}]] as a [B, 2, C, H, W] tensor, BUT if n is odd then the last image is doubled so we still have a pair of images and code is not crashing.
> 
> Is there any valid input to SuperGlue which involves one image? If not, then we can raise an exception when the image processor is called if the number of images isn't even

Made these changes and changed tests accordingly

> * For the concern you had about the naming for the model and output with ImageMatching, I suggest ImageKeypointMatching. And maybe we could rename KeypointDetection references to ImageKeypointDetection ?
> 
> `ImageKeypointMatching` sounds good to me! Or just `KeypointMatching`
> 
> We could, although renaming models should be done lightly as this would be a breaking change. I don't think the `Image` prefix adds much.

I've renamed ImageMatching occurences into KeypointMatching

I need to make a last pass on the code to check for quality, refactoring, naming and commenting problems in the code but overall I'd say we have everything



Hey @amyeroberts ,
I addressed the review comments.
I am sorry for the offence regarding the naming of the images of the Tower Bridge, but in my defense, I just copied it from the [original repo](https://github.com/magicleap/SuperGluePretrainedNetwork/tree/master/assets/phototourism_sample_images) 😬 

> I am sorry for the offence regarding the naming of the images of the Tower Bridge, but in my defense, I just copied it from the [original repo](https://github.com/magicleap/SuperGluePretrainedNetwork/tree/master/assets/phototourism_sample_images) 😬

@sbucaille No offence taken! It's a very common mistake 😄 

Hey @amyeroberts 
Addressed most of the issues, answered some of your confusions. There are still a few points to be addressed 😄 

Hi @amyeroberts 
Just made another pass on the code and tackled some remaining typing and docs issues.
I have created the custom image matching dataset to be used in the tests [here](https://huggingface.co/datasets/stevenbucaille/image_matching_fixtures), ready to be copied.
I also completed the model docs of both models of SuperGlue ([here](https://huggingface.co/stevenbucaille/superglue_outdoor) and [here](https://huggingface.co/stevenbucaille/superglue_indoor), also ready to be copied I think. 
Let me know if there is still things to do

Hey @amyeroberts , just a gentle bump here
Also, as my LightGlue's implementation is coming close to the SuperGlue's state in terms of progress, I've created a [PR](https://github.com/huggingface/transformers/pull/31718) for it

Hey @amyeroberts 
I've done a pass on the suggestions, let me recap it here since quite a lot of code changed : 
- the `concat`, `stack` and `batch` functions for the hidden states and attention probabilities have been refactored together.
- the list of matching variables have been removed, the detection is done on all the images first, then we compute the maximum number of keypoints found in these images, which tells us the required dimensions for the matching outputs, which we fill as we are matching each images together.
- the `SuperGlueImageProcessor` have been reworked to cover all the failing input use cases you mentionned. This part might need some refinement but a dedicated test have been wrote to be as exhaustive as possible.
- all inconsistencies on variable names have been taking care of, let me know if you still find one.
- SuperGlue docs have been updated to be more clear and to the point. Also got rid of opencv

Last question I asked in the review comments is about dividing SuperGlueModel and SuperGlueForKeypointMatching. From the different more traditional models like ViT or other feature extractors, such model is necessary when we can do multiple tasks with the model like classification, segmentation etc, based on the same features. But here, SuperGlue "is already" a specific task we apply to SuperPoint. I don't know what could someone do with just the output of the GNN ? And even if they want it, they can retrieve it using `output_hidden_states=True`. Is it more out of convention to have such separated model or what is the motivation behind ?

Have a great week-end !

@amyeroberts gentle bump here with a simple rebase on main.

Hey @amyeroberts ! Comments have been addressed, let me know what you think !

Hi @sbucaille, thanks for working on this PR and sorry for a long waiting! I will review it this week :slightly_smiling_face: 

Hi @qubvel , thanks for taking the lead on this PR. Good luck on catching up on all the comments that were made 😅 but one I remember that requires particularly your attention would be this one : https://github.com/huggingface/transformers/pull/29886#discussion_r1633768800
The dataset I created for testing needs to be cloned into `hf-internal-testing`

Hi @qubvel , while I address the comments, let me answer your question and raise others : 
> 1. Can this model be used with any other keypoint detection model without finetunining? If so, maybe it's better to decouple SuperGlue from SuperPoint and just provide keypoints/scores to `preprocessor` and then to `model.forward`.

SuperGlue has only been trained on top of the frozen SuperPoint model. Other models like LightGlue also got trained on top of the frozen SuperPoint, but also [have versions trained on top of frozen DISK](https://github.com/cvg/LightGlue/releases/tag/v0.1_arxiv) (another keypoint detector model). So keypoint matchers in general are "dependent" on the keypoint detector they were trained with. Other architectures include detection and matching at the same time, like [LoFTR](https://zju3dv.github.io/loftr/) or [EfficientLoFTR](https://github.com/zju3dv/EfficientLoFTR) which do not require a keypoint detection model but will fit in the keypoint matching task with images as input instead of keypoints. 
For reference, there also exists [XFeat](https://github.com/verlab/accelerated_features) (a relatively fast keypoint detector) where authors provided a smaller version of LightGlue (same architecture but fewer parameters) as a keypoint matcher.
So, based on that, and of course correct me if I'm wrong, I'm not sure we should have SuperPoint and SuperGlue decoupled. Also, from a user standpoint, do we want them to instantiate two models to perform image matching ? Or do we want them to either chose between the two tasks, keypoint detection or matching ? the first will provide keypoints and the second will provide matching, but both take images as inputs. Also, the current implementation does not prevent user from using the match_image_pair which takes as input the keypoints/score you mention.
Let me know if you have more questions
 
> Overall, it looks great! High-quality PR, and it was a pleasure to review the code!

Thanks !

An additional note, I've mentionned it to Amy earlier, but this PR depends on the SuperPoint fix PR #33200 for two reasons:
- The output of SuperPoint is changed, so I need to reflect that in the SuperGlue model
- I've worked on a similar `post_process_keypoint_matching` method to transform the masked matching outputs so it can be used more easily like with SuperPoint's outputs (this is already ready for review, it just requires the other PR to be merged)


Hi @sbucaille!

Thanks for the answer, I see now that we are using `descriptors` from SuperPoint, which means we are model-dependent. In that case, it's the correct way to include SuperPoint inside the SuperGlue. My bad, I overlooked it previously, thanks for the clarifications!

Regarding https://github.com/huggingface/transformers/pull/33200, I hope it will be merged soon!

Also, I think `post_process_keypoint_matching ` should be included in this PR to superglue the image processor to have a full model inference pipeline ready with the merge!

Hi @qubvel ,
  Lot of changes in the last commits, lots of your comments disappeared from the GitHub review page so I'll address them here :
- Good job for the new conversion script standard, I've updated mine, it's way cleaner ! I've changed the nn.Module to the actual SuperGlue models except for the old one which is required to convert the weights
- I've reverted the fusion of batchnorm into linear
- About the model itself, I've added batching, the code is cleaner, I've got rid of the concat, stack and batch ragged pairs functions since we don't need it anymore with the built-in masking
- Since batching allows different images inputs to have the same shape thanks to padding, I've fused many of the method's inputs to reduce the number of it (so less keypoints0, keypoints1 arguments in signatures)
- I renamed the q, k, v and out layers and changed from a single MLP to a list of MLP as suggested
- With all these changes, on my GPU, I have the inference speed improved from 174ms for 3 pairs to match to 94ms !
- In the ImageProcessor, about the big function to process the input and validate the format, I understand that the code gives headaches (even though @amyeroberts managed to greatly clean it from my first try 😄), but I couldn't manage to make it "simpler", logically speaking, the code is longer, but at least reading it should be easier. The problem with this function is that there are a lot of valid inputs of different formats (I've listed them in the docs), so checking if the input falls into any of them gives a lot of ifs. I tried to replace the repetitive statements with explicit function names. Also, this function needs to transform any of the input into a single list of images for the following steps of the processor to work (resizes, etc). So the strategy I adopted is to transform each kind of valid input, whether it is a list of image or a sequence of list of images and turn it into a list of image, and then check a last time if the output is valid (because there are edge cases slipping through, we can discuss this more in details if you want, don't hesitate to send me a message on Slack). 

Edit : There is a problem with a newly introduced test that I can't seem to address myself, `test_batching_equivalence`  checks the differences between batches and single row input. In my case, matching_scores is the same tensor, but full of 0's, which gives a cosine similarity of 0, hence the error in the tests.

Let me know if you have any questions or if I missed review comments but I think I've addressed all of them

PS: Let me know when you've copied the dataset I mentionned in https://github.com/huggingface/transformers/pull/29886#issuecomment-2389705243 so I can update the code

Hi @qubvel ,
Since https://github.com/huggingface/transformers/pull/33200 has been merged, I just pushed the changes regarding the post process for keypoint matching with SuperGlue.
Last thing to add is some changes to docs for which I'll probably ask @merveenoyan some feedback to get something visually appealing 😄 

Hey @qubvel 
Thanks for the review !
I indeed misinterpreted your first comments about the use of nn.Modules in the convert script. I've changed it to edit the statedict instead.
About the Attention module, I used the copy mechanism to use the Bert implementation. Although a portion of these classes is not used (head pruning for example), do you agree this was what you meant with Amy and I didn't misinterpreted your comments ? 
Let me know what you think of these changes, I fixed the other issues in the meantime.

Hi @qubvel 
Thanks for the clarification !

About the conversion script, I've changed it to comply with the approach you enumerated. Although, since some layers have been tweaked in the resulting model compared to the original implementation (such as the keypoint encoder being a list of MLP's instead of one big MLP), I had to add a step that deals with layers indices logic that can't simply be put into a conversion map (that's why I initially used these classes actually). Other than that I hope the conversion script now meets the standard.

About the attention, SuperGlue uses cross attention, the original just implement it as a self attention with reverted arguments, but I applied BERT attention using its cross attention mechanism by feeding `encoder_hidden_states` and `encoder_attention_mask` when cross attention is needed and it works like a charm. I think we can keep BERT as the copied class, unless I'm not aware of other factors where you would prefer CLIP to be used, I can always change it.

Thanks again for your help and I'd be glad to know what you think of my last changes !

Hi @qubvel thanks for the review ! Just committed changes to address your comments.
About the tests, `test_batching_equivalence`, I've mentionned it in an earlier message but it fails because SuperGlue returns `matching_scores` as a Tensor full of zeros. But in the tests, the model falls in the case described in [`test_modeling_common.py:755`](https://github.com/huggingface/transformers/blob/13493215abceafc1653af88b045120014fb4c1fc/tests/test_modeling_common.py#L754) which makes use of cosine sim with zeros which gives 0, hence the test failing. Should I rewrite the test in `test_modeling_superglue` ?
About `SuperGlueModelIntegrationTest::test_inference`, I can't replicate the failure 🤔
On an additional note, let me highlight this comment made by @amyeroberts https://github.com/huggingface/transformers/pull/29886#discussion_r1624523321 where the [dataset](https://huggingface.co/datasets/stevenbucaille/image_matching_fixtures) I use for the tests should be copied into [`hf-internal-testing`](https://huggingface.co/hf-internal-testing).
Also, I've uploaded the weights to [`magic-leap-community`](https://huggingface.co/magic-leap-community) org and updated references to HF weights in the code.


Thanks for highlighting the dataset transfer issue, I duplicated it to https://huggingface.co/datasets/hf-internal-testing/image-matching-test-dataset, please update in tests.

> About the tests, test_batching_equivalence, I've mentionned it in an earlier message but it fails because SuperGlue returns matching_scores as a Tensor full of zeros. But in the tests, the model falls in the case described in [test_modeling_common.py:755](https://github.com/huggingface/transformers/blob/13493215abceafc1653af88b045120014fb4c1fc/tests/test_modeling_common.py#L754) which makes use of cosine sim with zeros which gives 0, hence the test failing. Should I rewrite the test in test_modeling_superglue ?

Is it expected that SuperGlue returns a tensor full of zeros? Can we modify inpus to avoid this? If it is expected and cannot be avoided then we can just write a new test for this model overriding the existing `test_batching_equivalence`.

> About SuperGlueModelIntegrationTest::test_inference, I can't replicate the failure 🤔

Ok, might be cuda/torch difference, lets see if it would be fine in CI. To run CI please make an empty commit with the message `[run-slow] superglue`. The message should be exactly like this, otherwise the CI will not trigger slow tests.


> Thanks for highlighting the dataset transfer issue, I duplicated it to https://huggingface.co/datasets/hf-internal-testing/image-matching-test-dataset, please update in tests.

Tests have been updated

> > About the tests, test_batching_equivalence, I've mentionned it in an earlier message but it fails because SuperGlue returns matching_scores as a Tensor full of zeros. But in the tests, the model falls in the case described in [test_modeling_common.py:755](https://github.com/huggingface/transformers/blob/13493215abceafc1653af88b045120014fb4c1fc/tests/test_modeling_common.py#L754) which makes use of cosine sim with zeros which gives 0, hence the test failing. Should I rewrite the test in test_modeling_superglue ?
> 
> Is it expected that SuperGlue returns a tensor full of zeros? Can we modify inpus to avoid this? If it is expected and cannot be avoided then we can just write a new test for this model overriding the existing `test_batching_equivalence`.

SuperGlue returns tensors as big as the input, in terms of number of keypoints. No matching is equivalent of a `-1` index and a score of `0`. But changing that may not fix the problem since by implementing batching we introduced padded tensors that may be filled with zeros for the sake of other matching. Overriding the `test_batching_equivalence` test would, I guess, consist of copying it and just tweaking the several lines that don't apply to SuperGlue as linked in my previous message. Let me know if it still sounds good to you and I'll apply this.

> To run CI please make an empty commit with the message `[run-slow] superglue`. The message should be exactly like this, otherwise the CI will not trigger slow tests.

Ok I thought it would just consist of the `[run-slow]` tag 😄 it should run now 



> Overriding the test_batching_equivalence test would, I guess, consist of copying it and just tweaking the several lines that don't apply to SuperGlue as linked in my previous message. Let me know if it still sounds good to you and I'll apply this.

Sounds good 👍 

The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_29886). All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

Hi @sbucaille, slow `test_inference` test is also failing in CI

<img width="985" alt="Screenshot 2024-11-18 at 09 55 36" src="https://github.com/user-attachments/assets/e2b98270-0e60-44d0-907a-c125da9ce6cb">


We use the following in CI:
 - GPU: Tesla T4
 - torch==2.5.1+cu121
 
Let me know if you can reproduce it

Hi @qubvel 
> <img alt="Screenshot 2024-11-18 at 09 55 36" width="985" src="https://private-user-images.githubusercontent.com/31920396/387164395-e2b98270-0e60-44d0-907a-c125da9ce6cb.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzE5MjQzMTQsIm5iZiI6MTczMTkyNDAxNCwicGF0aCI6Ii8zMTkyMDM5Ni8zODcxNjQzOTUtZTJiOTgyNzAtMGU2MC00NGQwLTkwN2EtYzEyNWRhOWNlNmNiLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDExMTglMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQxMTE4VDEwMDAxNFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWRmYWYzMWNhMDZlN2VkMTY4ZDA5NDMzMTM0YjY0ODRlYzA5ZWU3NzI5YjEwY2EzZTA5Mzk2YWE2ZDE2ZDdhM2QmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.M1fPnvukt8owv5pXWIOmkNUgfz-VcAeqlKhPsIEzJ9g">
> We use the following in CI:
> 
> * GPU: Tesla T4
> * torch==2.5.1+cu121
> 
> Let me know if you can reproduce it

I can't seem to reproduce the result, although I managed to reproduce the results you shared yesterday, with a predicted shape of `[2, 2, 865]` instead of an expected `[2, 2, 866]`, but I can't manage to get the value `[2, 2, 867]` returned by the CI.
IIRC, from the original SuperPoint PR to the fix PR, values returned by the model had changed. I don't know if you often have the case with other models but if SuperGlue is that vulnerable to CUDA or torch changes, maybe we could use the cosine similarity function with a threshold value for such very small shifts in values ?


> IIRC, from the original SuperPoint PR to the fix PR, values returned by the model had changed. I don't know if you often have the case with other models but if SuperGlue is that vulnerable to CUDA or torch changes, maybe we could use the cosine similarity function with a threshold value for such very small shifts in values ?

Yes, we definitely have to make it a bit more robust in terms of torch/gpu/cuda change, otherwise, our CI will fail with the next update. Sounds good regarding the threshold.

I changed the tests. One thing I realized is the `matching_threshold` parameter in the SuperGlueConfig is set to 0.2 by default which makes matches close to the value 0.2 to suddenly change from a match to a non-match, which makes the checks in the test much more varying. 
Either way I changed the tests to use cosine similarity with a tolerance of `0.1` for the actual matches and `0.01` for the scores. The motivation behind these values are the following : 
- `0.1` : for actual matches, if a match becomes a non-match, having a value change radically from any number to -1 has a big impact on the cosine similarity between two 30 long vectors of integers.
- `0.01` : different from matches, matching_scores are values between 0 and 1.

About the value of `866` being either `865` on our setups or `867` in the CI is not necessarily a value we should check in SuperGlue's tests since it reflect the number of keypoints detected by SuperPoint. Although I added a tolerance in terms of matches found in total by the model.
Let me know what you think of these changes (also I replied to your review comment about the superpoint `do_grayscale` argument)


Sorry missed it for review today, will have a look tomorrow morning! 

Btw, test is still failing
```
>       self.assertTrue(
            expected_number_of_matches - expected_number_of_matches * number_of_matches_tolerance
            < predicted_number_of_matches
        )
E       AssertionError: False is not true
```

Maybe we have to set something like `abs(expected_number_of_matches - predicted_number_of_matches) <= 1` or ` <= 2`, as we know there might be slight diff + add a comment on that in the code (I did a really quick look, ignore if it is not relevant 😄 )


Hi @qubvel,
No worries, I changed strategy for the tests and went for checking each value individually whether or not they were close to the expected, but let a tolerance of only 1 being different possible. I can increase the value if necessary, I don't know how strict you would like this test to be. I also added a comment explaining the problem with references to this PR. Let me know what you think


Hi @qubvel 

Huge thanks for checking on the model with other outputs, it took me a bit of time but I could investigate on the model's outputs.
Turns out the tensor manipulations for attention steps are different in the original implementation (see [this commit](https://github.com/huggingface/transformers/pull/29886/commits/c31e8e073650da0392d1266dd9b025a3e1bec6d9)), the 256-long features were cut into [64, 4] tensors instead of [4, 64] which changed the whole scores and attention probabilities computation. Unfortunately, to match original implementation results, I can no longer use the Copied from statement.
I've run the model on the pair of images you used and got the following result : 

![matched_image](https://github.com/user-attachments/assets/aaf77ca1-2400-4343-aacc-1668600a2123)

I also ran it again on the image pair I used in the docs and got such a difference I feel ashamed I didn't notice the problem right away 😅 

Before :
![image/png](https://cdn-uploads.huggingface.co/production/uploads/632885ba1558dac67c440aa8/01ZYaLB1NL5XdA8u7yCo4.png)

After :
![matched_image](https://github.com/user-attachments/assets/123fa540-329c-42b2-a969-b017bcedbb21)

I've updated the tests and applied the changes you suggested in your review. I also increased the tolerance for CUDA variances by 1 since it didn't pass last tests.

Thanks for investigating it! And no worries about not noticing the problem, that's why a review is needed 😄 

Regarding attention modifications, is it possible to transpose the tensor outside the attention block to keep "#Copied from"? Otherwise, I feel like it's better to revert to your own simplified attention implementation. Sorry for asking for these back-and-forth modifications.

I'm afraid I can't perform this step outside the attention class or I don't know how to do it. The first view/permute happens after the query, key, value layer forward and the second happens after obtaining the context layer. The second could be extracted but I don't think it can be the case of the first. Permuting before the qkv layers alters the qkv output :(

> I'm afraid I can't perform this step outside the attention class or I don't know how to do it. The first view/permute happens after the query, key, value layer forward and the second happens after obtaining the context layer. The second could be extracted but I don't think it can be the case of the first. Permuting before the qkv layers alters the qkv output :(

Ok, can you please revert to the previous attention module? I mean not bert's one

Hi @qubvel ,
I partly reverted the changes. I removed the copy from statement and removed all the lines that were necessary for the copy but not for SuperGlue (prune_heads, positional embedding, dropout, etc...). This keeps a consistency with other models implementations in terms of code, if anyone would need to maintain this code but is used to BERT and other's implementations, then it will be easier for them.
I anticipated a problem by increasing the tolerance in the tests as they were not passing on my CUDA 12.6 setup yesterday night.
Let me know what you think !

EDIT: Also made the change about `do_grayscale` in SuperPoint and SuperGlue image processors, since do_grayscale is set to True in all models on Hub

Hey @qubvel ,

Just added the fixes and answered your comments !

@ArthurZucker please review whenever you have bandwidth!

Some notes:
1) The model is just for inference, no training is available. 
2) It is designed to use the SuperPoint model (and only it for now) for points and descriptors extraction. 
3) We tried to utilize the standard `transformers` attention module with `#Copied from` but some custom transpose ops are required.
4) The conversion script is adjusted to follow `mllama` format, `__init__` files are also updated.

Colab notebook for running the model:
https://colab.research.google.com/drive/1NhwofZFzy7IMN4irN-jC-9LZy7dx_GZ2?usp=sharing&authuser=3#scrollTo=DYyoTAMRljBM

Checkpoints:
https://huggingface.co/magic-leap-community/superglue_outdoor
https://huggingface.co/magic-leap-community/superglue_indoor

@sbucaille please correct me if I am wrong or add if I missed something!

@qubvel nothing to add on the notes ! I've made the last change about the `matching_threshold` parameter, updated the configs and docs on the Hub. I also removed some typos and errors here and there that I noticed. @ArthurZucker 's fresh eye will be useful to catch the potential last ones 😄 

@qubvel , @ArthurZucker ,
I took the time to think again about this problem of transpose ops and managed to find a solution. 
The original SuperGlue implemention made it impossible to match the standard library implementation of attention because the forwarded hidden states were not organised in a correct manner, which required us to use `.reshape(batch_size, num_keypoints, head_size, num_heads).permute(0, 3, 1, 2)` instead of the desired `.reshape(batch_size, num_keypoints, num_heads, head_size).permute(0, 2, 1, 3)` on `query`, `key` and `value` in `transpose_for_scores` method. This means that if we look at the indices of our hidden state, given that the desired hidden state would look like `[0, 1, 2, 3, 4, 5, 6, 7]` with 2 heads, the features returned by the q, k and v layers look like this `[0, 4, 1, 5, 2, 6, 3, 7]`.
By applying the appropriate transpositions to the weights of the model, I managed to make the q, k and v linear layers return hidden states in the desired form which makes the model now compatible with the standard attention implementation.

I re-reverted the changes on SuperGlueAttention classes to make it a copy of BERT. Weights are updated on hub, @qubvel let me know what you think, I made some additions to the convert script. If everything looks good, I have nothing else to add so we can proceed to the final @ArthurZucker 's review



Awesome! Great job, @sbucaille! That is absolutely aligned with Transformers to have weights transposed during conversion to match the standard implementation. Thanks a lot for taking the time to look deeper into this issue!

Please push run-slow to make sure we have all tests in a good shape.

@qubvel done !

Hi @qubvel , @ArthurZucker ,
Just pushed a small commit about a tiny bug I found and just fixed in the post processing method. The program would crash as outputs may be on a difference device from CPU where `image_sizes` are instantiated as tensors.

Thanks, @sbucaille! Arthur is on/off this week, but hopefully, he will be able to review it then. Thanks for your patience 🤗

Hey @ArthurZucker ! Gentle 🎅 bump here, happy holidays !

Hey @qubvel ! Wish you all the best for this new year ! Heard some good news about vision PR merges so here is a gentle bump ! 😬  

@sbucaille happy new year as well 🤗 

Hi @ArthurZucker , happy new year to you too and thanks for the review !
I've addressed and replied to your comments.
On the `validate_and_format_image_pairs` function being too big, I've removed cases where we have 4D or 5D arrays and added parameterized tests.
For the other comments, let me know what you think !

Congratulations @sbucaille on the model merged 🎉 🎉 🎉  Fantastic work! Thanks for iterating so many times to follow our standards 🤗