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.

Sign up for GitHub

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

Jump to bottom

VariableLayer, use add_batch_axis=False by default #763

Closed
albertz opened this issue Nov 24, 2021 · 4 comments · Fixed by #764
Closed

VariableLayer, use add_batch_axis=False by default #763

albertz opened this issue Nov 24, 2021 · 4 comments · Fixed by #764
Labels
potential-new-behavior Discussions about RETURNN behaviour

Comments

@albertz
Copy link
Member

albertz commented Nov 24, 2021 •
edited
Loading

This is probably an historic artifact and should never be needed.

Similar as #630 and #639.

This maybe needs a new behavior version (#508). Or maybe not, as it should (in theory) have no impact on anything...
@albertz albertz added the potential-new-behavior Discussions about RETURNN behaviour label Nov 24, 2021
@Zettelkasten
Copy link
Member

This needs a new behavior version (#508).

I wonder, is this really needed? I don't think there are many layers which really operate on this axis and that make sense to use after VariableLayer. Or are there still layers that require a batch axis to be present?

@albertz
Copy link
Member Author

albertz commented Nov 24, 2021

Hm yea, maybe you are right. I don't know. I guess I just check if all our tests are passing, and if so, then it is fine.

@Zettelkasten
Copy link
Member

Further, if this is really doesn't break anything, we could also go as far and just remove support for add_batch_axis and add_time_axis entirely, just as we did in #641 for RangeInAxisLayer.

@albertz
Copy link
Member Author

albertz commented Nov 24, 2021

Further, if this is really doesn't break anything, we could also go as far and just remove support for add_batch_axis and add_time_axis entirely, just as we did in #641 for RangeInAxisLayer.

In #641 where we removed keepdims in RangeInAxisLayer, this was safe as it was anyway broken in the code (as far as I remember), so no working config could possible exist and get broken by this change.

This is maybe different here. It has worked before, so any config with explicit add_batch_axis=True would have worked before and would break then.

Although, there are probably not much (or any) configs which do have that. I don't know.

So this might need a new behavior version.

albertz added a commit that referenced this issue Nov 24, 2021
@albertz
VariableLayer, change default add_batch_axis to False
618b708 
Fix #763.

As this should not have any influence on behavior,
this does not need a new behavior version.
@albertz albertz mentioned this issue Nov 24, 2021
Merged
albertz added a commit that referenced this issue Nov 24, 2021
@albertz
VariableLayer, change default add_batch_axis to False
44f456f 
Fix #763.

As this should not have any influence on behavior,
this does not need a new behavior version.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
potential-new-behavior Discussions about RETURNN behaviour
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

VariableLayer, change default add_batch_axis to False rwth-i6/returnn
2 participants
@albertz @Zettelkasten