Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 IoU3D as a custom c++ op (CPU) #890
Add IoU3D as a custom c++ op (CPU) #890
Changes from 33 commits
6bcac82
87bcb8e
2c87da0
3112a03
3b92be3
b26f9d7
99b3b92
da958cf
734f74d
a25796d
2143176
3b10e4a
3b707b1
6085f7a
4d62a16
b91ba90
72a4f11
0157c4e
9d74837
2b1fa02
6468eb7
8ac5bfe
46f8200
081fa29
da49210
3b9aa15
146a959
870e795
982d3ba
eb585bc
c4a7fa6
f7f6b73
b0b4263
131872a
2b138d9
8a54df4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some high level question on this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is adapted from TFA. I'm adding a link to the original script and a brief blurb about what this does.
We don't necessarily need to ship binaries for Windows and Mac OS, but including this makes it a lot easier for people to install from source it they want. I'm not sure if it makes sense to offer pre-built wheels for these environments, as I agree that it does add maintenance cost for not a ton of benefit. But it doesn't cost much to support it in the setup script IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose that these things are ported from Addons:
https://github.com/tensorflow/addons/blob/master/build_deps/build_pip_pkg.sh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pain is to support those if TF drops pre-built wheels for them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question with build_pip_pkg.sh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a little doc blurb at the top -- is there anything else specifically you're looking for here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the header file inclusion and libtensorflow_framework inclusion seems correct to me.
Though it'd be better to point out where we're relying this from. The main reason for this ask is that "tensorflow_framework" might not be a guaranteed binary in the long run, and we should know how to fix this once that becomes true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow -- do you mean comments specifying that this is enabling linking to the installed TF version, or something else?