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 RepeatVector layer #139

Merged
merged 15 commits into from
Jun 24, 2021
Merged

Conversation

dosier
Copy link
Contributor

@dosier dosier commented Jun 18, 2021

No description provided.

Copy link
Contributor Author

@dosier dosier left a comment

Choose a reason for hiding this comment

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

Commits 78196a8 and 6347d65 can be ignored. They are reverted in 208cd56

Copy link
Contributor

@avan1235 avan1235 left a comment

Choose a reason for hiding this comment

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

I leave m thoughts, don't worry about them a lot but try to get from them as much as you can as in my opinion this code needs most of these fixes but by doing this you can learn creating cleaner code

@avan1235
Copy link
Contributor

Commits 78196a8 and 6347d65 can be ignored. They are reverted in 208cd56

Just FYI your commits are squashed before merge so I think the reviewer looks only on the final version of commit so the steps that you did are not so important - the final result counts only 😉

@dosier
Copy link
Contributor Author

dosier commented Jun 20, 2021

@avan1235 thanks a lot for your comments. This is my first time contributing to an open source project and your comments were really thoughtful, will make changes and keep in mind for future contributions.

- added missing require check in init block of RepeatVector
- updated docs
- reformatted code
- housekeeping
- change require message in computeOutputShape
- used inputShape.size(...) for creating shape
- removed author tag
Copy link
Collaborator

@zaleslaw zaleslaw left a comment

Choose a reason for hiding this comment

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

Please fix a few minor things

}

// TODO: generalise this for Layer, see https://github.com/JetBrains/KotlinDL/issues/145
private operator fun RepeatVector.invoke(input: Array<FloatArray>): Output<Float> = Ops.create().let { tf ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe you could make a util internal extension function for the Layer class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe I could, u mean as described in #145 ?
What should I do for now though, remove the function?

Copy link
Collaborator

Choose a reason for hiding this comment

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

So at this moment, you could leave it as is, I suppose.
The improvement related to #145 should be implemented in the separate PR and discussed with other contributors faced with this problem.

@zaleslaw zaleslaw added the Review This PR is under review label Jun 23, 2021
@dosier dosier requested a review from zaleslaw June 24, 2021 07:42
@zaleslaw zaleslaw added LGTM PR reviewed and is ready to merge and removed Review This PR is under review labels Jun 24, 2021
@zaleslaw zaleslaw merged commit 34ae33f into Kotlin:master Jun 24, 2021
@zaleslaw zaleslaw linked an issue Jun 24, 2021 that may be closed by this pull request
5 tasks
@dosier dosier deleted the feature/RepeatVectorLayer branch July 10, 2021 04:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM PR reviewed and is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add RepeatVector layer
4 participants