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

Better way to specify axis for layers operating on rec time axis #781

Closed
Zettelkasten opened this issue Nov 26, 2021 · 1 comment · Fixed by #780
Closed

Better way to specify axis for layers operating on rec time axis #781

Zettelkasten opened this issue Nov 26, 2021 · 1 comment · Fixed by #780

Comments

@Zettelkasten
Copy link
Member

Zettelkasten commented Nov 26, 2021

This is a special case of #597.

I noticed that for #776 in WindowLayer, but also for e.g. CumsumLayer that operate on the rec time axis, we have a check like this:

    if axis == "T" and data.time_dim_axis is None:
      # Assume inside RecLayer.
      # (then the code for a single step)
    else:
      axis = data.get_axis_from_description(axis)
      # (then the code for all steps at once, operating on `axis`)

This is flawed as soon as the input has two dynamic dims: Then the input might have a time_dim_axis even in the recurrent case, and the layer will silently behave very wrongly.

Instead, I would propose to allow axis to be a DimensionTag. Then, we can just check axis == network.get_rec_parent_layer(inside_loop=False).time_dim_tag.
Would we also want to allow the stag:description syntax here?
Also, in #391 for CumConcatLayer, we briefly thought of allowing :i as axis name. Maybe allow this too?

I worked on this a month ago, for CumsumLayer, see #780. But I never tested this, it was just something a colleague of mine stumbled across but then gave up on. Now with #776, and dim tags becoming popular, I remembered to raise this issue here.

@albertz
Copy link
Member

albertz commented Nov 26, 2021

Yes, I agree that this should be done better. See also my comments on your PR. I think with consistent dim tags, we can simply do the check like axis == network.get_inside_rec_time_dim(). This should be exactly the recurrent case (when axis is a DimensionTag).

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 a pull request may close this issue.

2 participants