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

Generalize MinMax monotonic optimizer #25330

Merged

Conversation

lgeiger
Copy link
Contributor

@lgeiger lgeiger commented Jan 30, 2019

This PR generalizes the MinMax monotonic optimizer to support SegmentMax, UnsortedSegmentMax and ArgMax operations.

This will improve performance of such operations after monotonic function.

@Harshini-Gadige Harshini-Gadige self-assigned this Jan 30, 2019
@Harshini-Gadige Harshini-Gadige added size:M CL Change Size: Medium awaiting review Pull request awaiting review labels Jan 30, 2019
@Harshini-Gadige Harshini-Gadige added this to Assigned Reviewer in PR Queue via automation Jan 30, 2019
lgeiger added a commit to lgeiger/tensorflow that referenced this pull request Jan 30, 2019
This PR adds `Asin`, `Atan`, `Softsign` and `Softplus` to the collection of elementwise monotonic operations. This allows the arithmetic optimizer to optimize more cases.

Continuation of tensorflow#25330
PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Jan 30, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jan 30, 2019
@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Jan 31, 2019
@Harshini-Gadige Harshini-Gadige added ready to pull PR ready for merge process and removed awaiting testing (then merge) labels Jan 31, 2019
@tensorflow-copybara tensorflow-copybara merged commit bc695da into tensorflow:master Feb 1, 2019
PR Queue automation moved this from Approved by Reviewer to Merged Feb 1, 2019
tensorflow-copybara pushed a commit that referenced this pull request Feb 1, 2019
@lgeiger lgeiger deleted the general_min_max_monotonic branch February 1, 2019 09:46
lgeiger added a commit to lgeiger/tensorflow that referenced this pull request Feb 1, 2019
This PR adds `Acos`, `Acosh`, `Asin`, `Atan`, `QuantizedRelu`, `QuantizedRelu6`, `QuantizedReluX`, `Softsign` and `Softplus` to the collection of elementwise monotonic operations. This allows the arithmetic optimizer to optimize more cases.

Continuation of tensorflow#25330
lgeiger added a commit to lgeiger/tensorflow that referenced this pull request Feb 1, 2019
This PR adds `Acos`, `Acosh`, `Asin`, `Atan`, `Softsign` and `Softplus` to the collection of elementwise monotonic operations. This allows the arithmetic optimizer to optimize more cases.

Continuation of tensorflow#25330
@lgeiger
Copy link
Contributor Author

lgeiger commented Feb 1, 2019

Thanks for the fast merge! You might want to check out #25332 too, which adds support for more monotonic ops.

lgeiger added a commit to lgeiger/tensorflow that referenced this pull request Feb 1, 2019
This PR adds support for max pooling operations to the algorithmic optimizer for monotonic functions.

This follows up on tensorflow#25330
/cc @ezhulenev
tensorflow-copybara pushed a commit that referenced this pull request Feb 5, 2019
@lgeiger
Copy link
Contributor Author

lgeiger commented Feb 6, 2019

@ezhulenev Can you tell me the reason why this PR was reverted in 0507dba?

It would be great to get feedback so I can fix it.

@lgeiger lgeiger restored the general_min_max_monotonic branch February 6, 2019 00:21
@ezhulenev
Copy link
Member

@lgeiger it failed in https://gist.github.com/ezhulenev/0b8ab39e1533933197a61f23675e719b

AssertionError: 0.5 != 0.90499997 within 2 places

Sorry didn't have time to dig into it myself, had to rollback to unblock other teams. Probably something simple.

lgeiger added a commit to lgeiger/tensorflow that referenced this pull request Feb 6, 2019
This PR adds support for max pooling operations to the algorithmic optimizer for monotonic functions.

This follows up on tensorflow#25330
/cc @ezhulenev
@lgeiger
Copy link
Contributor Author

lgeiger commented Feb 6, 2019

No worries, thanks for the link. I'll take a look.

@lgeiger
Copy link
Contributor Author

lgeiger commented Feb 6, 2019

It seems like my changes are fine but surfaced a bug in the existing algorithmic optimizer.
I made a PR to fix this: #25535

I'll reopen this PR after #25535 and #25438 are merged or rejected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes ready to pull PR ready for merge process size:M CL Change Size: Medium
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

7 participants