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

[A; B] and [A B] concatenation syntax (#187) #209

Merged
merged 7 commits into from
May 22, 2017
Merged

Conversation

oxinabox
Copy link
Collaborator

does #187
So that cool.

This could do with some code review (like all code).
It contains a @goto which is frowned upon by Dijkstra.
Normally I want to use gotos to break out of nested loops, and then I immediately see that I want to refactor to be using a function and then can use return to breakout.

But this time I want to restart the loop from the beginning
One option would be to use a while loop and manage the index manually
But I don't think that is more readable.

One thing that is still pending is a general way for this to be dispatched to when one or more of the varargs is an AbstractTensor and one or more is not (but assumably can be converted).
Right now this is accomplished by specialcasing the first two arguments.
So long as one of the first two arguments is an AbstractTensor then it is dispatched to correctly.
I think that covers the vast majority of uses.

Maybe it is worth expanding to first 3? Beyond that doing it manually becomes cumbersome.
I could perhaps workout a metaprogramming loop and do it to the first dozen or so.


Not sure what to do with hvcat yet.
I don't really know where it is used.

@codecov-io
Copy link

codecov-io commented Apr 28, 2017

Codecov Report

Merging #209 into master will increase coverage by 0.9%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #209     +/-   ##
=========================================
+ Coverage   62.92%   63.83%   +0.9%     
=========================================
  Files          48       48             
  Lines        3361     3473    +112     
=========================================
+ Hits         2115     2217    +102     
- Misses       1246     1256     +10
Impacted Files Coverage Δ
src/ops/transformations.jl 90.55% <100%> (+1.59%) ⬆️
src/py.jl 0% <0%> (ø) ⬆️
src/shape_inference.jl 83.49% <0%> (+0.16%) ⬆️
src/core.jl 85.76% <0%> (+1.69%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0988ad7...6f77034. Read the comment docs.

@oxinabox oxinabox changed the title [A; B] and [A B] concatenation syntax [A; B] and [A B] concatenation syntax (#187) Apr 29, 2017
@oxinabox oxinabox mentioned this pull request May 11, 2017
49 tasks
@oxinabox
Copy link
Collaborator Author

TODO:
check that vcat(foo)==foo and hcat(foo)==foo'

I suspect they are errors right now.

@oxinabox
Copy link
Collaborator Author

With that corner case closed,
I would very much like this to be reviewed and merged, (@malmaud @MikeInnes)
and ideally a new Tensorflow.jl version tagged.

We are using this syntax part in TensorFlowDiffEq.jl (@ChrisRackauckas)

And it is hard to deploy tests against non-tagged versions.

Copy link
Collaborator

@MikeInnes MikeInnes left a comment

Choose a reason for hiding this comment

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

LGTM

@oxinabox oxinabox merged commit 348550d into master May 22, 2017
@oxinabox oxinabox deleted the ox/concat_syntax branch May 22, 2017 10:01
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