Fix KL computation for dof != 1

pavlin-policar / openTSNE

Extensible, parallel implementations of t-SNE

https://opentsne.rtfd.io

BSD 3-Clause "New" or "Revised" License

1.42k stars 157 forks source link

Fix KL computation for dof != 1 #246

Closed dkobak closed 11 months ago

dkobak commented 11 months ago

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

dkobak commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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

dkobak commented 11 months ago

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 https://github.com/pavlin-policar/openTSNE/pull/249. Could you please rebase this onto the latest master, and then hopefully, everything passes.

— Reply to this email directly, view it on GitHub https://github.com/pavlin-policar/openTSNE/pull/246#issuecomment-1684912416, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACEN75Z2STE2H2RTWFJF26LXWCHNRANCNFSM6AAAAAA27WIVAQ . You are receiving this because you authored the thread.Message ID: @.***>

pavlin-policar commented 11 months ago

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 commented 10 months ago

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 commented 10 months ago

No worries, thanks for finding the fix!

dkobak commented 10 months ago

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

pavlin-policar commented 7 months ago

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