Fix KMM coefficient and add tests

[xukai92]

No description provided.

[juliohm]

I think the previous version is the correct one. I double checked in the textbook. Unless the textbook contains a typo in which case we need to double check more carefully.

Could you please elaborate on the change?

[xukai92]

That was actually a hotfix. I commited a new one which makes more sense. Basially we missed the braces.

[juliohm]

Oh, the braces causing trouble again. So we were multiplying the matrix entries before solving the system, that is an issue :)

Please feel free to merge, that script I shared in another closed issue can serve to debug it further.

[xukai92]

Do you think we should actually add the test script to our test suite?

[juliohm]

That would be great. Could you please do that following the organization of the other tests for KLIEP for example? There are some visual regression tests there that you could copy/paste with KMM.

…

On Mon, Dec 30, 2019, 12:26 Kai Xu ***@***.***> wrote: Do you think we should actually add the test script to our test suite? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#19>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZQW3IGCVR4P6D4KUMSA4TQ3IHJ5ANCNFSM4KBATJPQ> .

[xukai92]

I will do another round of clean-up for tests before merging.

[juliohm]

Maybe we should separate the indentation changes from the changes to fix KMM? Could you please submit separate PRs? One containing the fix to KMM + tests, and another considering aesthetic changes?

[xukai92]

It's a bit annoying to change it back to 2 spaces and change it back to 1 tab again. In fact, you can simply set "1 tab = 2 spaces" in your editor and things would like normal for you.

[juliohm]

It is difficult to review these changes in the future, specially if future contributors come in. Can you just create two separate branches, one with the identation changes that you can copy/paste from this branch, and another branch with just the fixes for the KMM? That would be helpful.

The problem is not the editor visualization, but the mix of changes of different nature in a single PR. I understand it is annoying but it is for the best.

[xukai92]

OK I can do this.

[xukai92]

I reverted most of the aesthetic changes. You are right, it makes the PR much clear!

I don't have more to add. Shall we merge it given CI passes?

[juliohm]

Can I take a look at it after the holiday? :) it should be fine. I'm just wondering if GitHub still has the functionality to merge all commits into a single one without the noise. Currently organizing details for the reveillon. By the way happy 2020! 🎉☺

…

On Tue, Dec 31, 2019, 11:25 Kai Xu ***@***.***> wrote: I reverted most of the aesthetic changes. You are right, it makes the PR much clear! I don't have more to add. Shall we merge it given CI passes? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#19>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZQW3PRBOHWVYGW4VANEXDQ3NI4BANCNFSM4KBATJPQ> .

[xukai92]

Can I take a look at it after the holiday? :) it should be fine.

Sure.

I'm just
wondering if GitHub still has the functionality to merge all commits into a
single one without the noise.

I believe you can just squash the commits.

By the way happy 2020! 🎉☺

Yes! Happy new year!

[juliohm]

Had a chance to review the PR. Please consider some minor modifications in the tests before we can merge and keep working on other features.

[xukai92]

@juliohm Did you have a chance to take another look of this PR?

[juliohm]

Sorry for the delay to review @xukai92 , didn't know you had pushed more changes. Below you can find further review comments.

[xukai92]

Shall we merge this?

[juliohm]

Let me take yet another look at the changes. I am still confused with some proposed changes in the PR.

[juliohm]

Sorry for the long review process. I didn't think we would take so long to get this in the right shape. To be honest, I would just break this PR into small PRs with very clear purpose. There is a mix of changes in tests, and in the KMM tha are imcompatible in a single PR.

[xukai92]

I reverted basic.jl and the tests should be there now. Sorry for the confusion again. I indeed deleted some when merging with the master!

[juliohm]

Thank you for improving the PR. 💯

I will merge it with the squashing feature of GitHub, and we can keep working on improving KMM in future PRs.