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

MergeDimsLayer should use keep_order=True (and disallow keep_order=False) #654

Closed
albertz opened this issue Sep 15, 2021 · 2 comments · Fixed by #784
Closed

MergeDimsLayer should use keep_order=True (and disallow keep_order=False) #654

albertz opened this issue Sep 15, 2021 · 2 comments · Fixed by #784
Assignees
Labels
potential-new-behavior Discussions about RETURNN behaviour

Comments

@albertz
Copy link
Member

albertz commented Sep 15, 2021

This is to follow the principle that the order of axes should never matter.
(keep_order is about using the user specified order in the layer axes argument, and ignoring the incoming data axes order.)

This probably needs a new behavior version (#508).

@albertz albertz added the potential-new-behavior Discussions about RETURNN behaviour label Sep 15, 2021
@albertz
Copy link
Member Author

albertz commented Sep 15, 2021

I now realize that this will also require consistent and unique dim tags (#632) because sth like "axes": "static" would merge all static axes, but again the order is not well defined here. So sth like this would be disallowed. You would have to use dim tags and specify it like "axes": [dim_tag_1, dim_tag_2] or so.

@albertz
Copy link
Member Author

albertz commented Nov 27, 2021

I now realize that this will also require consistent and unique dim tags (#632) because sth like "axes": "static" would merge all static axes

This is already the case right now.

    if keep_order:
      assert isinstance(axes, (tuple, list)), "%s: unique axes %r required" % (self, axes)
      axes_ = []
      for axis in axes:
        axis_ = self.input_data.get_axes_from_description(axis, allow_int=False)
        assert len(axis_) <= 1, "%s: unique axes %r required, but got %r -> %r" % (self, axes, axis, axis_)
...

albertz added a commit that referenced this issue Nov 27, 2021
Fix #654.

Also introduce Data.get_axes_from_description "dim:%i" variant. This is mostly as a simple way to fix many of the test cases. But I guess it could also be useful for the user in general.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
potential-new-behavior Discussions about RETURNN behaviour
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant