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

Support ver 7 #193

Merged
merged 8 commits into from
May 30, 2018
Merged

Support ver 7 #193

merged 8 commits into from
May 30, 2018

Conversation

fumihwh
Copy link
Collaborator

@fumihwh fumihwh commented May 24, 2018

I am not sure to use _limited_broadcast and _np_broadcast is a good idea.
They changed broadcast mechanism to as same as numpy from since version 7.

Also fix the tile version, from 1 to 6. 1 is not implemented yet.

@fumihwh fumihwh requested a review from tjingrant May 24, 2018 01:10
# Conflicts:
#	doc/support_status.md
#	onnx_tf/opset_version.py
@tjingrant
Copy link
Collaborator

Huh, so if I understand correctly, since TF conforms with NP style broadcasting, we don't need to do anything special about broadcasting in version 7 right?

@tjingrant
Copy link
Collaborator

On a separate note, can you comment why we should use cls.make_node instead of the helper.make_node from ONNX (i.e., here

). I don't have any problem with it, but we may want to document design choice as we go along because we may forget them in the future.

@fumihwh
Copy link
Collaborator Author

fumihwh commented May 29, 2018

Yes, we don't need _explicit_broadcast to calculate anymore from version7.

Copy link
Collaborator

@tjingrant tjingrant left a comment

Choose a reason for hiding this comment

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

Make sense, thanks!

@fumihwh fumihwh merged commit 516e986 into onnx:master May 30, 2018
@fumihwh fumihwh deleted the support-ver-7 branch May 30, 2018 07:03
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.

2 participants