6973 sincos pos embed

[NoTody]

Fixes #6973

Description

Adding support for sincos positional embedding for monai.networks.blocks.patchembedding.PatchEmbedding class.

This pull request corresponds to this opened issue #6973

Types of changes

• Non-breaking change (fix or new feature that would not break existing functionality).
• Breaking change (fix or new feature that would cause existing functionality to change).
• New tests added to cover the changes.
• Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
• Quick tests passed locally by running ./runtests.sh --quick --unittests --disttests.
• In-line docstrings updated.
• Documentation updated, tested make html command in the docs/ folder.

[NoTody]

The checks don't pass because they don't include timm package. Do I need to write these functions I used from timm? I included timm in requirements-dev.txt.

[wyli]

Thanks for the PR! looks like only minor utility functions are from timm, perhaps we can drop those for now instead of introducing a new dependency?

Also the argument renaming makes sense, ideally we should keep them backward compatibility as well because it's been used in many places https://github.com/search?q=org%3AProject-MONAI+pos_embed&type=code. For example PatchEmbeddingBlock(patch_embed="conv") should still work in the same way before and after this PR; and module instance PatchEmbeddingBlock(pos_embed="none", patch_embed="conv") can use the model checkpoint trained before this PR with PatchEmbeddingBlock(patch_embed="conv")

[NoTody]

Thanks for the PR! looks like only minor utility functions are from timm, perhaps we can drop those for now instead of introducing a new dependency?

Also the argument renaming makes sense, ideally we should keep them backward compatibility as well because it's been used in many places https://github.com/search?q=org%3AProject-MONAI+pos_embed&type=code. For example PatchEmbeddingBlock(patch_embed="conv") should still work in the same way before and after this PR; and module instance PatchEmbeddingBlock(pos_embed="none", patch_embed="conv") can use the model checkpoint trained before this PR with PatchEmbeddingBlock(patch_embed="conv")

Ok, I can just rewrite these two functions I used from timm.

For the second point, how should I make it backward compatible? Originally, you used pos_embed='conv' to represent patch_embed='conv'. Do you want to remain the same naming and introduce another name for pos_embed='none' or should I deprecate it?

[wyli]

Sure, another way would be marking the original 'pos_embed' as deprecating in monai v1.4.

MONAI/monai/utils/deprecate_utils.py

Line 123 in 6dfe17a

def deprecated_arg(

then add two options 'pos_embed_type' and 'proj_type' (or something similar), User provided 'pos_embed' is then interpreted as an equivalent of the latter.

[NoTody]

Sure, another way would be marking the original 'pos_embed' as deprecating in monai v1.4.

MONAI/monai/utils/deprecate_utils.py

Line 123 in 6dfe17a

def deprecated_arg(

then add two options 'pos_embed_type' and 'proj_type' (or something similar), User provided 'pos_embed' is then interpreted as an equivalent of the latter.

Hi, @wyli ,

I modified my pull request accordingly. I'm not sure why "premerge / packaging (pull_request)" don't pass on

Traceback (most recent call last):
  File "/tmp/tmp.hM3vB5K1Tk/tests/test_patchembedding.py", line 130, in test_ill_arg
    PatchEmbeddingBlock(
AssertionError: ValueError not raised

I tested it locally and it was able to raise the error. Could you help me to check?

[wyli]

Ok please sync up the tags to your fork... https://stackoverflow.com/questions/70678073/how-do-i-sync-tags-to-a-forked-github-repo I'll have a look soon.

[wyli]

/build

[wyli]

/build

[wyli]

/build

[wyli]

thanks, integration tests work fine now, merging for more model tests