Fix KL computation for dof != 1

[dkobak]

This should hopefully fix #238 and in addition implement exact KL computation with arbitrary dof.

[dkobak]

Apologies, this includes changes from #245 because I screwed up my git commits.

Also, the tests fail with fatal error: fftw3.h: No such file or directory and I don't know how this is related to the changes I did.

[pavlin-policar]

Wonderful, thanks for tracking down the bug! I guess I forgot to implement the ddof parameter in the positive gradient? So then I suppose the KL divergence was wrong in all the cases up until now? Also, I see two PRs now, and the other one seems to have only one commit. Should I close the other one and accept this one?

[dkobak]

Yes, this PR includes both changes -- I wanted to make two separate PRs but screwed something up with my git branches. The other one is not needed anymore.

But what's happening with the tests?

[dkobak]

I guess I forgot to implement the ddof parameter in the positive gradient? So then I suppose the KL divergence was wrong in all the cases up until now?

Yes. The gradient was implemented correctly, but the KL divergence was computed wrongly, and only gave the right answer for dof=1.

[pavlin-policar]

Yes, this PR includes both changes -- I wanted to make two separate PRs but screwed something up with my git branches. The other one is not needed anymore.

Okay, I guess we can close the other PR then.

I'm looking at the errors now, and I can't really see anything obvious. The fft.h error likely isn't really an error but just checks if we can use FFTW, and if not, falls back to numpys FFT implementation. It seems like the culprit is this one here:

TypeError: expected str, bytes or os.PathLike object, not get_numpy_include

Perhaps there was a recent change in pip or some other build tool, and now this trick that I use to dynamically resolve the numpy include path doesn't work anymore. It certainly has nothing to do with your change. I'll try to fix it ASAP.

[pavlin-policar]

I believe I've fixed this in #249. Could you please rebase this onto the latest master, and then hopefully, everything passes.

[dkobak]

Yes but I am traveling now so not sure how soon I'll be able to get to it. If you can do it yourself, then please go ahead.

…

On Sat, 19 Aug 2023, 12:20 Pavlin Poličar, ***@***.***> wrote: I believe I've fixed this in #249 <#249>. Could you please rebase this onto the latest master, and then hopefully, everything passes. — Reply to this email directly, view it on GitHub <#246 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACEN75Z2STE2H2RTWFJF26LXWCHNRANCNFSM6AAAAAA27WIVAQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[pavlin-policar]

Sorry for the delay, setting this up so that I can push to your PR is a pain, but hopefully, the tests pass now.

[dkobak]

Great, thanks a lot for taking care of it! And apologies that I could not fix it myself quickly enough. Only back to my computer now.

[pavlin-policar]

No worries, thanks for finding the fix!

[dkobak]

Could you tag it as 1.0.1 at some point? Would be cool if this bug fix gets into conda.

[pavlin-policar]

I'm sorry that this took so long. I've now released v1.0.1 with this fix.