Skip to content
This repository has been archived by the owner on Apr 28, 2023. It is now read-only.

Add a unit test for softmax #61

Merged
merged 1 commit into from Feb 24, 2018
Merged

Add a unit test for softmax #61

merged 1 commit into from Feb 24, 2018

Conversation

prigoyal
Copy link
Contributor

@prigoyal prigoyal commented Feb 23, 2018

we have a weird bug where we don't allow reusing the same variable twice

FAILS:
def softmax(float(N, D) I) -> (O, tmp) {
        tmp(n) max=! I(n, d)
        O(n, d) = exp(I(n, d) - tmp(n))
        tmp(n) +=! O(n, d)
        O(n, d) = O(n, d) / tmp(n)
}
ERROR: Func tmp cannot be given a new update definition, because it has already been realized or used in the definition of another Func. Aborted (core dump)

FAILS:
def softmax(float(N, D) I) -> (O, tmp, tmp1) {
        tmp(n) max=! I(n, d)
        O(n, d) = exp(I(n, d) - tmp(n))
        tmp1(n) +=! O(n, d)
        O(n, d) = O(n, d) / tmp1(n)
}
ERROR: Func O cannot be given a new update definition, because it has already been realized or used in the definition of another Func

PASSES:
def softmax(float(N, D) I) -> (O, expsum, maxVal) {
        maxVal(n) max= I(n, d)
        expsum(n) +=! exp(I(n, d) - maxVal(n))
        O(n, d) = exp(I(n, d) - maxVal(n)) / expsum(n)
 }

It throws error:

Func tmp cannot be given a new update definition, because it has already been realized or used in the definition of another Func. Aborted (core dump)

cc @jekbradbury

@abadams
Copy link
Contributor

abadams commented Feb 23, 2018

It's not necessary to go down to three lines of code, you just can't reuse names on the LHS once they've been used in another tensor. Otherwise the sizes of the tensors is ill-defined (in the general case - there's a solution in this case).

def softmax(float(N, D) I) -> (O, maxVal, expDistance, expSum) {
        maxVal(n) max=! I(n, d)
        expDistance(n, d) = exp(I(n, d) - maxVal(n))
        expSum(n) +=! expDistance(n, d)
        O(n, d) = expDistance(n, d) / expSum(n)
}

@prigoyal
Copy link
Contributor Author

yep, thanks for the clarification @abadams that makes sense, however, I am still wondering about the max=! issue and why that fails compilation

@zdevito can you take a look at that case, there's issue #63 filed as well

@prigoyal
Copy link
Contributor Author

CI is green, merging, simple test cases added

@prigoyal prigoyal merged commit 7ac7ed8 into master Feb 24, 2018
@prigoyal prigoyal deleted the softmax-test branch February 24, 2018 21:25
Copy link
Contributor

@nicolasvasilache nicolasvasilache left a comment

Choose a reason for hiding this comment

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

@prigoyal lots of dead code in this PR, please:

  1. enable and expect proper failure
  2. create issue (if not already existing)
  3. tag test with TODO(#numissue)

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants