DETR: changing position_embedding and key_value_position_embedding args

[Lorenzobattistela]

What does this PR do?

This PR refers to #19833 , and it just update some variables/docstrings names. Quoting the Issue, the paper mentions that the position_embeddings argument of the cross-attention layer are these input embeddings called object queries. And the key_value_position_embeddings is refered to as spatial_position_embeddings.

This PR is limited to DETR model.

Notes

This is my first contribution, so I'm happy to adjust anything in this PR. I ran all tests and style, and it went all, except for one:
make fixup. I got the following output:

Reading the output, I assume it is about other file using classes in modeling_detr. I'll wait for updates. I will also wait for review for doc updating or more guidance.

Fixes # (issue)

Before submitting

• This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
• Did you read the contributor guideline,
  Pull Request section?
• Was this discussed/approved via a Github issue or the forum? Please add a link
  to it if that's the case.
  Position embedding in the DETR model #19833
• Did you make sure to update the documentation with your changes? Here are the
  documentation guidelines, and
  here are tips on formatting docstrings.
• Did you write any new necessary tests?

Who can review?

@NielsRogge
@amyeroberts

[HuggingFaceDocBuilderDev]

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

[A-guridi]

Hi,

I dont know if this issue is still up.

I believe you need to change the names of the files mentioned in the fixup too. Since in the paper of Conditional DETR, they also use the same nomenclature (sometimes object queries are also called content queries though) .

For example in modeling_conditional_detr.py the names of the forward function are still position_embeddings, so you would need to change that to object queries for consistency.

Same applies to the other file mentioned in the fixup too. I am also new to fixing PRs in this repo, so I would leave this decision to the reviewers, but I believe it makes sense if you would like to apply the changes @Lorenzobattistela . If not, maybe another issue could be created for that.

[Lorenzobattistela]

@A-guridi Hey, I understand these names could be changed to keep consistency, and I am up to do this. But I don't know if this is the right to do since the issue is specific about DETR. But I'll try what you said, let's wait up the reviewers

[amyeroberts]

@NielsRogge Could you review and confirm if it aligns with your suggestion in #19833?

[NielsRogge]

Hmm this is confusing, we'd like to make the distinction between object queries (which are added to the inputs of each attention layer of the decoder) and spatial position embeddings (which are added to the keys and values of each cross-attention layer of the decoder)

[Lorenzobattistela]

yeah, it seems a little confusing, since I was assigning object_queries to a variable different than object_queries arg. I think I confused thing. It should be:

self.encoder_attn(
                hidden_states=hidden_states,
                object_queries=object_queries,  
                key_value_states=encoder_hidden_states,
                attention_mask=encoder_attention_mask,
                spatial_position_embeddings=query_position_embeddings,
                output_attentions=output_attentions,
            )

[NielsRogge]

Thanks for working on this, left some comments.

Specifically, DETR's decoder uses 2 types of position embeddings:

• the ones that are added to the inputs i.e. hidden states of each cross-attention layer (the object_queries)
• the ones that are added to the keys and values of each cross-attention layer (the spatial_position_embeddings)

[Lorenzobattistela]

Thanks for working on this, left some comments.

Specifically, DETR's decoder uses 2 types of position embeddings:

• the ones that are added to the inputs i.e. hidden states of each cross-attention layer (the object_queries)
• the ones that are added to the keys and values of each cross-attention layer (the spatial_position_embeddings)

working on it

[Lorenzobattistela]

git history got messed up, will open a new PR just with the correct changes

[Lorenzobattistela]

Reopened PR #24652