Skip to content

Transpose2d, Concatenate2d, and up to 3 Clones for io_stream #402

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

Merged
merged 5 commits into from
Jun 17, 2022

Conversation

jmduarte
Copy link
Member

@jmduarte jmduarte commented Sep 25, 2021

This PR has 3 independent, minor changes (I can split them into smaller PRs if needed):

  • Transpose2d for io_stream
  • Concatenate2d for io_stream
  • Clone streams up to 3 times (instead of just up to 2 times)
  • Add unit test for all 3

These features are needed, for example in this interaction network model: https://gist.github.com/jmduarte/9ea95df65feac29f9663c47a439e2d7e

@jmduarte jmduarte changed the title [WIP] Tranpose2d and Concatenate2d for io_stream [WIP] Transpose2d and Concatenate2d for io_stream Sep 25, 2021
@jmduarte jmduarte changed the title [WIP] Transpose2d and Concatenate2d for io_stream Transpose2d, Concatenate2d, and up to 3 Clones for io_stream Sep 26, 2021
@jmduarte jmduarte marked this pull request as ready for review September 26, 2021 01:44
@jmduarte jmduarte force-pushed the tranpose2d_concat2d_stream branch from e6d24f0 to b0e55c3 Compare October 1, 2021 00:57
@jmduarte jmduarte requested review from thesps and thaarres October 1, 2021 02:07
thesps
thesps previously requested changes Oct 1, 2021
@thesps
Copy link
Contributor

thesps commented Oct 1, 2021

Thanks for this, looks nice! To be clear, for the review, only the increase from 1 to 100 test examples is a requested change, the other items are for discussion.

I tested with the 100 examples and it still works fine. I ran cosim as well since the io_stream version could have some dataflow behaviour: all good there too.

Maybe something for the future: now we have layers to clone a stream 2 times and another to clone it 3 times. Can there be a CloneN solution?

@jmduarte
Copy link
Member Author

jmduarte commented Oct 1, 2021

@thesps I responded to your comments and added some follow-up questions.

We should probably have a CloneN solution, but it's not clear to me how to handle the HLS templates for that.

@jmduarte
Copy link
Member Author

@thesps @vloncar let me know if there are any other updates needed or if you have an idea of how to do the CloneN solution

@jmduarte
Copy link
Member Author

Added concatenate1d here to partially resolve #436

@jmduarte jmduarte linked an issue May 11, 2022 that may be closed by this pull request
@jmduarte jmduarte self-assigned this May 23, 2022
@jmduarte jmduarte force-pushed the tranpose2d_concat2d_stream branch from d1b8814 to c4baaed Compare June 17, 2022 03:36
@jmduarte
Copy link
Member Author

@thesps @vloncar rebased this PR and I think it's much cleaner now and good to go!

@vloncar vloncar dismissed thesps’s stale review June 17, 2022 16:26

Raised issues have been resolved

@vloncar vloncar merged commit b7bc153 into master Jun 17, 2022
@jmduarte jmduarte deleted the tranpose2d_concat2d_stream branch November 2, 2022 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

io_stream implementation of concatenate 1d and 2d Dense Layer with Linear activation causes clone stream later not working
3 participants