Skip to content
This repository has been archived by the owner on Aug 15, 2019. It is now read-only.

Fix argMax & argMin benchmarks #1174

Merged
merged 4 commits into from Aug 2, 2018
Merged

Fix argMax & argMin benchmarks #1174

merged 4 commits into from Aug 2, 2018

Conversation

davidsoergel
Copy link
Member

@davidsoergel davidsoergel commented Jul 18, 2018

Description

The reduction op benchmarks operate on a 2D tensor and expect a scalar response. However, argMax and argMIn reduce rank by only one dimension at a time. This fix simply applies these ops twice in a row in order to produce a scalar.

It may seem a little weird that we'll now measure time for two calls instead of one, but I think that's OK.

  • it doesn't matter as long as we consider this a relative measure,
  • this makes the measurement more comparable to the other reduction ops, which are reducing over both dimensions here, and
  • the time is likely dominated by the first call anyway.

Fixes tensorflow/tfjs#504

BUG


For repository owners only:

Please remember to apply all applicable tags to your pull request.
Tags: FEATURE, BREAKING, BUG, PERF, DEV, DOC, SECURITY

For more info see: https://github.com/tensorflow/tfjs/blob/master/DEVELOPMENT.md


This change is Reviewable

@nsthorat
Copy link
Contributor

Hmm -- just curious, what is the motivation for this change?

@davidsoergel
Copy link
Member Author

The motivation came from the linked bug. Running the benchmark on argMin or argMax currently crashes with Error: Number of coordinates in get() must match the rank of the tensor, because reduction_ops_benchmark:50 has op(input).get();, which only works if op(input) is a scalar.

Sure we could split the test into two (one for to-scalar reducers and one for to-rank-minus-one reducers), but this solution seemed more expedient.

@nsthorat
Copy link
Contributor

How about instead we just call ".data()" for reduction ops?

@nsthorat
Copy link
Contributor

Or, actually even better, call reduction ops on a vector instead of matrix?

@nsthorat
Copy link
Contributor

Gentle ping on this since I am also now seeing this.

@davidsoergel
Copy link
Member Author

Oops, sorry I lost track of this. Implemented the 1D approach.

Copy link
Contributor

@dsmilkov dsmilkov left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2.
Reviewable status: 0 of 1 approvals obtained (waiting on @nsthorat and @dsmilkov)

@dsmilkov dsmilkov merged commit 335eaf1 into master Aug 2, 2018
@dsmilkov dsmilkov deleted the fix-argmax-benchmark branch August 2, 2018 20:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants