Implementation of SuperPoint and AutoModelForKeypointDetection
Author: sbucaille
Pull request: yes

# What does this PR do?

This PR implements SuperPoint, one of the few models that generate keypoints and descriptors given an image, as discussed in [this previous pull request](https://github.com/huggingface/transformers/pull/25697)
The goal is to implement this model and a new type of AutoModel : `AutoModelForKeypointDetection` (name to be discussed).
This PR is also the replacement of a previous [PR](https://github.com/huggingface/transformers/pull/25786) for which the branch ended up being unusable.

## Who can review?

@amyeroberts @ArthurZucker @rafaelpadilla 

## TODO's

- [x] Implement SuperPointConfig and SuperPointModel as PretrainedConfig and PretrainedModel
- [x] Generate a conversion script for the original weights
- [x] Implement the new `AutoModelForKeypointDetection` mapping
- [x] Test the model
- [x] Write documentation



Comments:
Hi @amyeroberts @rafaelpadilla @ArthurZucker ,

I couldn't see other solution than to create a branch from scratch and re-implementing everything that was discussed since the beginning of the original [PR](https://github.com/huggingface/transformers/pull/25786). Here, most of my problems (RegNet related error when using `make fix-copies` which was caused by the fact that my environment was outdated after doing the merge, the `index.md` huge diff, and others...) are fixed.
As I said, I re-implemented everything that was in the other PR, and addressed all other points that were mentioned in reviews (such as docs) which of course may need more discussion. I tried to make sure everything point discussed were included. Also, I renamed `InterestPointDescription` to `KeypointDetection` as this term is more widely used in the literature.
Anyway, I wanted to implement this model in transformers and contribute, you told me it would take more time than the other method, I'll take all this mess I made myself as part of the learning path 😆
Let me know what you think, what should be the next steps, I believe we're almost there ! 🤗 

Steven

Hi @sbucaille, thanks for opening this PR and working from the previous reviews! 

At the moment, I don't see any tests implemented for the model or image processor. The next step would be adding those 🤗 

Looks like I forgot to `git add` the tests folder, fixed !
I also addressed the issue where the model was too big for the tests, I added a configuration for a smaller one and adjusted the other tests.

Hi @amyeroberts @ydshieh ,
I just looked again at the PR and noticed that there were conflicts due to recent changes to the `main`branch. So instead of making the merge directly through GitHub interface, I did what you advised me to do earlier, I updated my main branch and rebased this branch onto the main, and managed to resolve the conflicts (the implementation of StableLM ended up at the same lines SuperPoint is implemented in all the readme's and __init__'s), then I pushed the changes.
First question, is it normal that this PR lists all the other commits ?
Second question, if I merged this way, why is GitHub still complaining and asking me to merge manually through the interface like such : 
![image](https://github.com/huggingface/transformers/assets/24275548/7b3306f2-b34f-43a8-bd8b-c2a5c8aa8902)
In the end I feel like merging manually through the interface would have been faster. Am I missing something ?

Also, appears all the tests pass now 🤗 

Hi @sbucaille, regarding your questions about rebasing

> First question, is it normal that this PR lists all the other commits ?

No, it shouldn't. Did you force push after rebasing? It's necessary to do this, as rebasing is effectively rewriting the history of  the branch, 

> Second question, if I merged this way, why is GitHub still complaining and asking me to merge manually through the interface like such :

What do you mean by "the interface"? 

When performing a rebase, it might be necessary to resolve conflicts. What rebasing is trying to do is move the first commit of your branch to the head of main. However, as the branch was originally branched off an older commit, there might be conflicts if files in this branch make changes to files which have also been modified on `main`. It might also happen if you've also merged in `main` into this branch in the interim, as the branch now contains information about newer, upstream commits on `main`. Therefore, how to rewrite the history, so this branch and all its commits are from the tip of `main` isn't obvious. 

The easiest way to manage this is to rebase main into the branch often, have short-lived branches and avoid merge commits in the branch.

Here are some nice articles about rebasing which I found useful when first learning about it: 
* https://www.atlassian.com/git/tutorials/merging-vs-rebasing
* https://www.atlassian.com/git/tutorials/rewriting-history/git-rebase

@amyeroberts Alright I think I start to grasp the base and merge things, this PR is my first in any open source project and I knew in advance I would have been in front of such issues, so thank you for taking the time 🤗
As of now, is my PR still ok ? Should we continue to review the changes ? What are the next steps ?

@sbucaille I'd suggest first resolving the git history, so this branch only contains commits that are part of this PR. Once our house is tidy we can review and iterate on the PR. From first glance, things are looking good! Doing this now will be a lot easier than trying to do it in the middle of a review. 



@amyeroberts holy that was not easy but I think I understood the mechanism, I once more rebased the branch to include the latest commits. It seems I am now 4 commits ahead of main, which should be fine (until tomorrow morning where new commits will be pushed to main I guess).
So I guess it is better now to see a bunch of "force push" in the pull request than the list of all the commits from the main right ? 
How often do you suggest to do this manipulation ? You talked about short lived branches but I guess this does not apply to mine since I've been working on it for a couple of months right ?
Anyway, I'm ready to move on the last review process

@sbucaille Great! So main will continue to have lots of new commits. The history you're seeing isn't so much number of commits "ahead" of main, rather, the number of commits on top of a certain main commit, let's say ABC. When you rebase, you're re-writing so that the commits are now on top of the latest main commit, say EDF. It's kind of like shifting things along in history. 

You don't need to rebase every time there's new commits on main. The purpose of rebasing is to make sure your branch and main don't diverge and any changes made to the same file has no clashes and a clear order to apply commits. As well as including any important upstream changes e.g. bug fixes. It's basically the same as having a merge commit but a lot cleaner as only the branches commits are shown.

> So I guess it is better now to see a bunch of "force push" in the pull request than the list of all the commits from the main right ?

Yep! This just lets me know there's been a rewrite of the history. This is important as rebasing can also be used for editing the commits in the branch itself.

> How often do you suggest to do this manipulation ? You talked about short lived branches but I guess this does not apply to mine since I've been working on it for a couple of months right ?

It depends. If you touch a lot of common files, you'll want to do it regularly. Particularly in the final steps when getting ready to merge the branch in - at least once a day. This is a model PR, so probably doesn't need it quite so often. Now you've done the difficult rebase, all the rest should be pretty easy to do. In fact, I'd image there would be little to no conflicts. 

We'd like all branches to live as short as possible. This is less true for model PRs - but we still would like them to be resolved in the order of days/weeks wherever possible. 

> Anyway, I'm ready to move on the last review process

Great! Next step is to get all the tests passing on the CI.

@amyeroberts Seems right, now. Turns out I uncommitted certain files during my rebase, I'll need to practice a bit more in the future 😅 Tests are passing !

Hey @amyeroberts ,
I addressed most of the suggestions, although I was not sure about one of them, let me know what you think. I'm looking forward this merge ! 😄 🤗 

Hi @amyeroberts,
I addressed the last problems apart from the README one. I sent an email to the authors, waiting for their answer. But none of them still work at MagicLeap, in the case I don't have a positive answer from them and we can't have a repository owned by the company, what should we do ?

@sbucaille OK, great. I'll give a final review. I've created https://huggingface.co/magic-leap-community where we can store the checkpoints. I'll reach out to magic leap and ask if they want to take control of it, but we can use that in the meantime. 

If you share a link to all the checkpoints for the model, I can copy them across to this org and then you can update the checkpoints in the PR 

Hi @amyeroberts ,
I managed to get the contact of a potential individual that would be in charge of this, I can add you to the mail thread if you give me an email address.
Regarding the weights, you can either copy them from the repo [I initially created ](https://huggingface.co/stevenbucaille/superpoint/tree/main) or run the `convert_superpoint_to_pytorch.py` script provided in the superpoint module

@sbucaille Great! You can get me on my HF email: amy@huggingface.co

@sbucaille I've copied across the checkpoints - so they can be updated in the PR now. 

Could you open a PR on https://huggingface.co/magic-leap-community/superpoint to fill in the model card?

Hi @amyeroberts,  as you can probably see, I've added you to the mail thread and opened a pull request to the hub superpoint repo

Also, just rebased the branch and figured out main was not passing tests because of repo-consistency checks, I've added SuperPoint to the appropriate README's and will watch closely when main is updated to rebase once again with what it missing there

Hi @amyeroberts ,
I found the problem, a `'` was not properly escaped line 146 which causes the syntax highlighting to be completely messed for the remainder of the file. I fixed it in [this commit](https://github.com/huggingface/transformers/pull/28966/commits/84c62d78b0120bd553c41d389d4d27c2da7fd0bd) so now if you look at my README_fr.md file, there is no more red things.
Should I create a new issue with a new PR just for this line or should we push it along with SuperPoint ?

Apart from that, my branch is rebased on the recent commits, let me know what we should do !

@sbucaille Awesome work adding this. If you can get the checkpoints updated for today we might be able to have this model make the next release 💪 

Hi @amyeroberts ,
Checkpoints are updated, I also fixed deleted remains of SuperPointForInterestPointDetection, test_pr_documentation test now passes. I guess we're good ! 

@sbucaille Great! Last thing (seriously this time) - can you run the slow tests for the model and share the output? Once you can confirm they all pass I'll merge 

![image](https://github.com/huggingface/transformers/assets/24275548/146be8e0-ac24-460a-8c0a-4bfae6374ad4)

@amyeroberts Is a screenshot sufficient ? When moving from the previous pull request to this one I forgot to add the additional COCO image.
Also, I'm not very familiar with the autodoc things but what causes this issue ? 
![image](https://github.com/huggingface/transformers/assets/24275548/296ef1f0-cbb4-48ec-bd63-667fab86efb7)


> @amyeroberts Is a screenshot sufficient ?

Main thing I'd need to see is the end of the test run, which shows how many of the tests have passed / been skipped etc. (I. can't see the command being run, but make sure to set RUN_SLOW=1)

I've commented on the part causing the error for the doctests 

@amyeroberts

```
RUN_SLOW=1 pytest tests/models/superpoint/
================================================================ test session starts =================================================================
platform linux -- Python 3.10.12, pytest-7.4.4, pluggy-1.4.0
rootdir: /home/steven.bucaille/transformers
configfile: pyproject.toml
plugins: dash-2.16.1, xdist-3.5.0, hypothesis-6.99.8, timeout-2.3.1
collected 93 items

tests/models/superpoint/test_image_processing_superpoint.py .s...........                                                                      [ 13%]
tests/models/superpoint/test_modeling_superpoint.py s........ssss....sssssssss.............s......s....sss.s.....s.......ss....ssss.           [100%]

================================================================== warnings summary ==================================================================
venv/lib/python3.10/site-packages/_pytest/config/__init__.py:1373
  /home/steven.bucaille/transformers/venv/lib/python3.10/site-packages/_pytest/config/__init__.py:1373: PytestConfigWarning: Unknown config option: doctest_glob

    self._warn_or_fail_if_strict(f"Unknown config option: {key}\n")

tests/models/superpoint/test_modeling_superpoint.py::SuperPointModelTest::test_fast_init_context_manager
  /home/steven.bucaille/transformers/tests/test_modeling_common.py:465: FutureWarning: `torch.testing.assert_allclose()` is deprecated since 1.12 and will be removed in a future release. Please use `torch.testing.assert_close()` instead. You can find detailed upgrade instructions in https://github.com/pytorch/pytorch/issues/61844.
    torch.testing.assert_allclose(init_instance.linear.bias, expected_bias, rtol=1e-3, atol=1e-4)

tests/models/superpoint/test_modeling_superpoint.py::SuperPointModelTest::test_fast_init_context_manager
  /home/steven.bucaille/transformers/tests/test_modeling_common.py:468: FutureWarning: `torch.testing.assert_allclose()` is deprecated since 1.12 and will be removed in a future release. Please use `torch.testing.assert_close()` instead. You can find detailed upgrade instructions in https://github.com/pytorch/pytorch/issues/61844.
    torch.testing.assert_allclose(

tests/models/superpoint/test_modeling_superpoint.py::SuperPointModelTest::test_torchscript_output_attentions
tests/models/superpoint/test_modeling_superpoint.py::SuperPointModelTest::test_torchscript_output_hidden_state
tests/models/superpoint/test_modeling_superpoint.py::SuperPointModelTest::test_torchscript_simple
  /home/steven.bucaille/transformers/src/transformers/modeling_utils.py:4225: FutureWarning: `_is_quantized_training_enabled` is going to be deprecated in transformers 4.39.0. Please use `model.hf_quantizer.is_trainable` instead
    warnings.warn(

tests/models/superpoint/test_modeling_superpoint.py::SuperPointModelTest::test_torchscript_output_attentions
tests/models/superpoint/test_modeling_superpoint.py::SuperPointModelTest::test_torchscript_output_hidden_state
tests/models/superpoint/test_modeling_superpoint.py::SuperPointModelTest::test_torchscript_simple
  /home/steven.bucaille/transformers/src/transformers/models/superpoint/modeling_superpoint.py:475: TracerWarning: Iterating over a tensor might cause the trace to be incorrect. Passing a tensor of different shape won't change the number of iterations executed (and might lead to errors or silently give incorrect results).
    list_keypoints_scores = [

tests/models/superpoint/test_modeling_superpoint.py::SuperPointModelTest::test_torchscript_output_attentions
tests/models/superpoint/test_modeling_superpoint.py::SuperPointModelTest::test_torchscript_output_hidden_state
tests/models/superpoint/test_modeling_superpoint.py::SuperPointModelTest::test_torchscript_simple
  /home/steven.bucaille/transformers/src/transformers/models/superpoint/modeling_superpoint.py:249: TracerWarning: Iterating over a tensor might cause the trace to be incorrect. Passing a tensor of different shape won't change the number of iterations executed (and might lead to errors or silently give incorrect results).
    scores = scores[0][tuple(keypoints.t())]

tests/models/superpoint/test_modeling_superpoint.py::SuperPointModelTest::test_torchscript_output_attentions
tests/models/superpoint/test_modeling_superpoint.py::SuperPointModelTest::test_torchscript_output_hidden_state
tests/models/superpoint/test_modeling_superpoint.py::SuperPointModelTest::test_torchscript_simple
  /home/steven.bucaille/transformers/src/transformers/models/superpoint/modeling_superpoint.py:249: TracerWarning: Using len to get tensor shape might cause the trace to be incorrect. Recommended usage would be tensor.shape[0]. Passing a tensor of different shape might lead to errors or silently give incorrect results.
    scores = scores[0][tuple(keypoints.t())]

tests/models/superpoint/test_modeling_superpoint.py::SuperPointModelTest::test_torchscript_output_attentions
tests/models/superpoint/test_modeling_superpoint.py::SuperPointModelTest::test_torchscript_output_hidden_state
tests/models/superpoint/test_modeling_superpoint.py::SuperPointModelTest::test_torchscript_simple
  /home/steven.bucaille/transformers/src/transformers/models/superpoint/modeling_superpoint.py:484: TracerWarning: Iterating over a tensor might cause the trace to be incorrect. Passing a tensor of different shape won't change the number of iterations executed (and might lead to errors or silently give incorrect results).
    for last_hidden_state, keypoints in zip(last_hidden_state, list_keypoints)

tests/models/superpoint/test_modeling_superpoint.py::SuperPointModelTest::test_torchscript_output_attentions
tests/models/superpoint/test_modeling_superpoint.py::SuperPointModelTest::test_torchscript_output_hidden_state
tests/models/superpoint/test_modeling_superpoint.py::SuperPointModelTest::test_torchscript_simple
  /home/steven.bucaille/transformers/src/transformers/models/superpoint/modeling_superpoint.py:312: TracerWarning: torch.tensor results are registered as constants in the trace. You can safely ignore this warning if you use this function to create tensors out of constant variables that would be the same every time you call this function. In any other case, this might cause the trace to be incorrect.
    divisor = torch.tensor([[(width * scale - scale / 2 - 0.5), (height * scale - scale / 2 - 0.5)]])

tests/models/superpoint/test_modeling_superpoint.py::SuperPointModelTest::test_torchscript_output_attentions
tests/models/superpoint/test_modeling_superpoint.py::SuperPointModelTest::test_torchscript_output_hidden_state
tests/models/superpoint/test_modeling_superpoint.py::SuperPointModelTest::test_torchscript_simple
  /home/steven.bucaille/transformers/src/transformers/models/superpoint/modeling_superpoint.py:312: TracerWarning: Converting a tensor to a Python float might cause the trace to be incorrect. We can't record the data flow of Python values, so this value will be treated as a constant in the future. This means that the trace might not generalize to other inputs!
    divisor = torch.tensor([[(width * scale - scale / 2 - 0.5), (height * scale - scale / 2 - 0.5)]])

tests/models/superpoint/test_modeling_superpoint.py::SuperPointModelTest::test_torchscript_output_attentions
tests/models/superpoint/test_modeling_superpoint.py::SuperPointModelTest::test_torchscript_output_hidden_state
tests/models/superpoint/test_modeling_superpoint.py::SuperPointModelTest::test_torchscript_simple
  /home/steven.bucaille/transformers/src/transformers/models/superpoint/modeling_superpoint.py:487: TracerWarning: Converting a tensor to a Python boolean might cause the trace to be incorrect. We can't record the data flow of Python values, so this value will be treated as a constant in the future. This means that the trace might not generalize to other inputs!
    maximum_num_keypoints = max(keypoints.shape[0] for keypoints in list_keypoints)

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
==================================================== 65 passed, 28 skipped, 27 warnings in 4.19s =====================================================
```

I applied your changes on the doc

(thank you for requesting running slow tests @amyeroberts  and thank you for running the slow tests @sbucaille ❤️ !)

@amyeroberts I'm afraid the error is back :thinking: 

@sbucaille I was wrong about the fix then! I'll check out your branch and see if I can figure out what's happening 

> @sbucaille I was wrong about the fix then! I'll check out your branch and see if I can figure out what's happening

don't hesitate to ping me if you have limited bandwidth @amyeroberts 

@amyeroberts should I revert the previous change ?

@sbucaille For the code example? No, that also wasn't correct (I just missed it in my reviews 🙈 )

OK - new issue. This one I think is because there isn't a model entry added in `docs/source/en/_toctree.yml`

@amyeroberts Added it to _toctree, I believe this belongs to the vision section ?

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

@sbucaille Yep! And all passing now! Thanks for all your work on this - great to see this added to the library 🤗 

Yaaay finally ! :joy: Thanks for the opportunity and all the feedback provided during this long lasting PR, I learned a lot of things !
Time to implement SuperGlue now :sunglasses: :hugs: