Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support dim tag with static shape and dyn_size_ext #1139

Open
albertz opened this issue Oct 10, 2022 · 4 comments
Open

Support dim tag with static shape and dyn_size_ext #1139

albertz opened this issue Oct 10, 2022 · 4 comments
Labels
difficulty: medium good first issue Should be a good starting point to get familiar with RETURNN, hopefully not too hard.

Comments

@albertz
Copy link
Member

albertz commented Oct 10, 2022

dyn_size_ext means that it has different sequence lengths per batch entry (or whatever dyn_size_ext shape is).

Currently this implies a dynamic shape (None dimension), and vice versa, a dynamic shape (None dimension) implies that we have some dyn_size_ext.

We should remove this assumption, and allow a static shape together with dyn_size_ext. It's mostly already supported.

This is needed for example for TPU (see #1162) or also for TFLite or ONNX in some cases.

@albertz albertz added good first issue Should be a good starting point to get familiar with RETURNN, hopefully not too hard. difficulty: medium labels Oct 10, 2022
@albertz
Copy link
Member Author

albertz commented Oct 15, 2022

Note there is already Dim.is_dynamic but a lot of code is checking dimension is None directly. All such code should use is_dynamic instead.

Note that the is_dynamic implementation in principle might look like return self.dyn_size_ext is not None. However, this is probably not exactly correct, as we need to care about two special cases:

  • Dim not yet defined, and then used as out_spatial_dim or so. Then dimension is None to signal this is yet undefined, and also dyn_size_ext is None.
  • Dim is dynamic, but the dyn_size_ext is unknown yet.

We cannot really properly distinguish those cases.

@albertz
Copy link
Member Author

albertz commented Oct 15, 2022

We might need to introduce a new flag, specifically for this, like _is_dynamic or so.

albertz added a commit that referenced this issue Oct 15, 2022
albertz added a commit that referenced this issue Oct 15, 2022
Also fixes Dim.is_dynamic. Related: #1139

Also needed for #1143.
@albertz
Copy link
Member Author

albertz commented Oct 21, 2022

Note, I'm already continually fixing any dimension is None checks.

Note that those checks don't necessarily always map to is_dynamic. They sometimes also check whether the dim is defined/known yet. If that is the actual meaning (and you have to see that from the context), then it should be replaced by not is_dim_known. Or even not dim.is_dim_known() or (dim.dyn_size_ext and dim.dyn_size_ext.placeholder is None).

@albertz
Copy link
Member Author

albertz commented Oct 21, 2022

We also need to be careful in get_for_batch_ctx. In the case the dim is not yet defined, this should also not do anything. Otherwise a static dim cannot properly be set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: medium good first issue Should be a good starting point to get familiar with RETURNN, hopefully not too hard.
Projects
None yet
Development

No branches or pull requests

1 participant