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

Make gmm lower triangle params row-major #36

Open
awf opened this issue May 28, 2019 · 4 comments
Open

Make gmm lower triangle params row-major #36

awf opened this issue May 28, 2019 · 4 comments

Comments

@awf
Copy link
Contributor

awf commented May 28, 2019

The comment at https://github.com/awf/ADBench/blob/37f7f15137e5c8759785840917e4d773819bfddf/usr/awf/Julia/gmm3.jl#L14 says that the elements in the lower triangle are inserted row-major.

This is the sensible order for most of the languages we support, but it appears that Manual inserts them column-major, and when we check for consistency, this will bite.

I believe it's worthwhile to make the (nontrivial) effort to make Manual row-major.

@awf
Copy link
Contributor Author

awf commented May 29, 2019

I just checked the document, https://github.com/awf/ADBench/blob/0a4f34e371deec4c45d3654bca4260f996730622/Documents/ms.tex#L201-L204

Which is also column-major, so this is a significant change.

@tomjaguarpaw
Copy link
Collaborator

To check my understanding, we have

  • Julia: row-major
  • Manual: column-major
  • Document: column-major

Which one(s) are we going to change?

@awf
Copy link
Contributor Author

awf commented May 29, 2019

My proposal is to change all to row-major. But that actually disfavours Julia, which is col-major by default...
More thought needed.

@msdmkats
Copy link
Contributor

Yep, it just bit me. Apparently, the reason julia/zygote code was producing incorrect results was just that - it treated packed lower triangle as row-major. And for d = 2 it didn't matter, hence the correct results for that case. I do propose, however, to make everything column-major (already fixed julia/zygote pack/unpack code to do just that in my branch), because everywhere else (c+, python, doc) it is column-major and changing that would be a pain. On the other hand, if some AD tool could benefit from it being row-major, our architecture allows it to do the rearranging during non-time-measured prepare and output stages.

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

No branches or pull requests

3 participants