Skip to content

Conversation

@yonigozlan
Copy link
Member

What does this PR do?

Remove inheritance in efficient loftr tests (following #42327). Also fix source of the issue, KeypointMatchingOutput is different for each keypoint matching model, so rename it to model-specific names.

@yonigozlan yonigozlan requested review from molbap and vasqu November 24, 2025 17:05
@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

Copy link
Contributor

@molbap molbap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok for the renaming. So we remove test inheritance for now? I thought we were ok with that (in tests ONLY of course) at some point. OK either way

Copy link
Contributor

@vasqu vasqu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx for fixing

Re: #42365 (review) @molbap

Things got ugly and the side-effects are not worth it. See #42192 / #42247 so we should keep things strict and separate from each other

Comment on lines +1357 to +1359
loss = None
return EfficientLoFTRKeypointMatchingOutput(
loss=loss,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we always have a loss of None? It's a bit weird to me but maybe training here just isn't supported

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes just kept if for API consistency, but training isn't supported

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trusting you on this :D

@yonigozlan yonigozlan enabled auto-merge (squash) November 24, 2025 21:34
@github-actions
Copy link
Contributor

[For maintainers] Suggested jobs to run (before merge)

run-slow: efficientloftr, lightglue, superglue

@yonigozlan yonigozlan merged commit 9bd85b0 into huggingface:main Nov 24, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants