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

MMulTranspose not working correctly #6378

Closed
RSBat opened this issue Sep 5, 2018 · 9 comments

Comments

@RSBat
Copy link

commented Sep 5, 2018

@Charele

This comment has been minimized.

Copy link

commented Sep 5, 2018

Hi,I take a test, but My result is good, so it has no problem.
code:
val mmt1 = MMulTranspose.builder().transposeResult(true).build()
val op1 = new Mmul(x1, x2, x3, mmt1)
Nd4j.getExecutioner().exec(op1)
println(x3)
println(x1.mmul(x2).transpose())

println("\nNext is not same:")
println(x1.mmul(x2))

result:
[[ 5.0000, 4.0000, -4.0000],
[ -14.0000, -18.0000, -33.0000],
[ 8.0000, 17.0000, 15.0000]]
[[ 5.0000, 4.0000, -4.0000],
[ -14.0000, -18.0000, -33.0000],
[ 8.0000, 17.0000, 15.0000]]

Next is not same:
[[ 5.0000, -14.0000, 8.0000],
[ 4.0000, -18.0000, 17.0000],
[ -4.0000, -33.0000, 15.0000]]

@RSBat

This comment has been minimized.

Copy link
Author

commented Sep 5, 2018

That is interesting, here is my test:

    val corr = A.mmul(B).transpose()
    val inc = A.mmul(B, MMulTranspose.builder().transposeResult(true).build())
    println(corr)
    println(inc)

and output (notice 2 extra outputs)

[[    0.9971,    0.8702,    0.5991,    0.7694], 
 [    0.5037,    0.0328,    0.3219,    0.3488], 
 [    0.3968,    0.4706,    0.0827,    0.3462]]
[[    0.0680,    0.7961], 
 [    0.8453,    0.3887], 
 [    0.3242,    0.1800], 
 [    0.1271,    0.2039]]
[[    1.0953,    0.2107,    0.4956], 
 [    1.3968,    0.5428,    0.5844]]
[[    1.0953,    1.3968], 
 [    0.2107,    0.5428], 
 [    0.4956,    0.5844]]
@Charele

This comment has been minimized.

Copy link

commented Sep 5, 2018

Hmm, I think it's a bug.
The mistake is in mmul() implement method of BaseNDArray.java
(MMulTranspose.java is good),

the detail:
@override
public INDArray mmul(INDArray other, MMulTranspose mMulTranspose) {
MMulTranspose mMulTranspose1 = MMulTranspose.builder()
.a(this)
.b(other)
.transposeA(mMulTranspose.isTransposeA())
.transposeB(mMulTranspose.isTransposeB())
.transposeResult(mMulTranspose.isTransposeResult())
.build();
System.out.println(mMulTranspose1.getA()); // why println here? typo ?
System.out.println(mMulTranspose1.getB());
return mMulTranspose1.getA().mmul(mMulTranspose1.getB());
}

When it return result, it don't consider "isTransposeResult", so the you get the bad result,
It should be fix like here:

public INDArray mmul(INDArray other, MMulTranspose mMulTranspose) {
    MMulTranspose mMulTranspose1 = MMulTranspose.builder()
            .a(this)
            .b(other)
            .transposeA(mMulTranspose.isTransposeA())
            .transposeB(mMulTranspose.isTransposeB())
            .transposeResult(mMulTranspose.isTransposeResult())
            .build();       
    
    INDArray ret = 
    		mMulTranspose1.getA().mmul(mMulTranspose1.getB());
    
    return mMulTranspose1.isTransposeResult() ? ret.transpose() : ret;
}
@saudet

This comment has been minimized.

Copy link
Member

commented Sep 6, 2018

@Charele Good catch, please send a pull request!

@RSBat

This comment has been minimized.

Copy link
Author

commented Sep 6, 2018

@saudet did you look at the code I highlighted?
Here's what is wrong: this.transposeA is set to some value, then this value is overwritten inside if, then this value is overwritten again outside if making that if almost useless. That is definitely not the way code should be written

@saudet

This comment has been minimized.

Copy link
Member

commented Sep 6, 2018

@RSBat No, I didn't write that code. If you have a suggestion though, please do send it as a pull request yes. It will make it easier to have it reviewed by the right person.

@saudet

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

@agibsonccc @maxpumperla Could you take a look at this?

@maxpumperla

This comment has been minimized.

Copy link

commented Sep 12, 2018

alright, I'll have a look. thanks

@AlexDBlack AlexDBlack assigned AlexDBlack and unassigned maxpumperla Sep 17, 2018

AlexDBlack added a commit that referenced this issue Sep 17, 2018
AlexDBlack added a commit that referenced this issue Sep 18, 2018
Various fixes (#6450)
* #6401 transposei fix

* #6378 MmulTranspose fix + cleanup

* #6442 validate array order

* Cleanup and test fixes

* #6403 batch norm validation

* #6389 Fix TransferLearning nOutReplace issues; add nInReplace method

* Trigger CI
@lock

This comment has been minimized.

Copy link

commented Oct 18, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Oct 18, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.