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

add initial nnet_constructor code #66

Merged
merged 4 commits into from
Jun 10, 2022
Merged

Conversation

JackTemaki
Copy link
Contributor

Related to #63

This is initial code I used for testing returnn_common integration, but we can change this as we see fit.

@albertz
Copy link
Member

albertz commented Jun 10, 2022

What is the goal of this PR? Already get it to a working stable state? Or just have some first code, and we will directly work in the dir then? Any future work without PRs, until we declare it as stable? Why is this PR needed at all then? Why did you not just push it directly to master?

@JackTemaki
Copy link
Contributor Author

Already get it to a working stable state?

The PR message clearly says that this is not the aim, but just for testing.

Any future work without PRs, until we declare it as stable? Why is this PR needed at all then? Why did you not just push it directly to master?

I do not understand. There should be no changes to common without PRs. I specifically added a README.md that says that the code is not "stable" and might still be altered, but nevertheless there should be no pushes without PRs, as of course I (and Benedikt and you as well I gues) will actively link setups against this code then.
Maybe I could have skipped the PR for the "first" code, by why should I? With a PR people better see what is happening and get notifications if enabled.

@albertz
Copy link
Member

albertz commented Jun 10, 2022

Already get it to a working stable state?

The PR message clearly says that this is not the aim, but just for testing.

It says "initial code" but it was not clear what this means. Runnable and stable? Totally broken? If there are no constraints other than "any code", then I don't see the purpose of having it as a PR.

Any future work without PRs, until we declare it as stable? Why is this PR needed at all then? Why did you not just push it directly to master?

I do not understand. There should be no changes to common without PRs.

Why? You also push to your user dir without PR. This is just the same here as long as we declare it as WIP-code.

I specifically added a README.md that says that the code is not "stable" and might still be altered, but nevertheless there should be no pushes without PRs,

Why?

as of course I (and Benedikt and you as well I gues) will actively link setups against this code then. Maybe I could have skipped the PR for the "first" code, by why should I? With a PR people better see what is happening and get notifications if enabled.

So I don't exactly know what you want know.

You want that we see the code changes? We don't need it to be a PR for that. You can look at the commits.

You want that we discuss it here and work on the PR or custom code branch? Until what point? Until it is all stable? This is unclear to me.

Copy link
Contributor

@Atticus1806 Atticus1806 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of it looks good, a few typos etc. and also added some suggestions.

:param name: name of the data (equivalent to the extern_data entry)
:param available_for_inference: if this data is available during decoding/forward pass etc...
:param dim_tags: list of dim tags representing an axis of the data, without batch or a hidden sparse dim
:param sparse_dim: provide this dim to make the data sparse and define
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and define the dim? I think this sentence is incomplete.

common/setups/returnn_common/nnet_constructor.py Outdated Show resolved Hide resolved
common/setups/returnn_common/nnet_constructor.py Outdated Show resolved Hide resolved
common/setups/returnn_common/nnet_constructor.py Outdated Show resolved Hide resolved
common/setups/returnn_common/nnet_constructor.py Outdated Show resolved Hide resolved
@JackTemaki JackTemaki merged commit 5ebf6c3 into main Jun 10, 2022
christophmluscher pushed a commit to christophmluscher/i6_experiments that referenced this pull request Jun 20, 2022
@JackTemaki JackTemaki deleted the nick_add_nnet_constructor branch October 11, 2022 17:02
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 this pull request may close these issues.

3 participants