-
Notifications
You must be signed in to change notification settings - Fork 6
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
TODOs in HordeAd.Core.DualNumber about defining more derivatives of common functions #39
Conversation
Thank you very much, Staszek! CI fails. In most of the cases, that's because your formula for However, not all failures are clear, so let's try to understand them before deciding on a fix. A couple of failures are probably due to my sparse matrix experiment not coping well with matrices of undetermined size (as your The last and embarrassing case is where a test says only "wrong result". It would be helpful to make it print any details. |
Right, and it also contains division (
Not that I understand, but I just eliminated
Fixed the test to print out more info, on my machine it now produces:
which, again, is a subtle numerical error. But in which function and which number is closer to the truth?... BTW why do those tests here on Github take 40 minutes to complete? Can we improve that? |
Not sure what to do with |
I'm not assuming that. However, if I knew one of them is more accurate (or faster) and if I had to guess which, I'd assume mine, just because it's repeated on ML blogs, while yours is derived from calculus. Ultimately, checking the quality of our pre-defined functions is a separate task, so I wouldn't worry and settle on something uniform and cheap (e.g., not requiring a change of tests). Probably, avoiding division by zero is a good idea, too, but not at a too high complexity cost (I guess
Actually, what I had in mind was to change
to
and this looks better to me, because it underlines that we are not using a bilinear function cosh (D u u') = D (cosh u) (dScale (sinh u) u') is even clearer to me, because it tell me what is the primal component and what is the dual component of the dual number (the derivative that, indeed, is a small expression tree). Please ask more, if what I said requires further clarification.
Thank you for the fix. What is truth? I'd recommend to change the test to expect one of two results: the result you get at home and the result CI obtains with the same test (if it differs). Ignore the old results; they are close enough, so we probably haven't made an error.
How much do they take on your machine? We can surely solve #14 and #20, but they are really hard (and #14 is almost finished). |
BTW, I've just merged a big PR to master. Apologies for the conflicts and for, probably, some extra boilerplate TODOs to do in DualNumber.hs. The code on master is now hlint-clean except for a dozen of cases. You can use hlint to tell you which parens are unnecessary. But feel free to ignore or to silence via |
Thank you for the boilerplate. I guess that's all the relevant TODOs? Could you rebase your branch instead of merge, to keep git history clean? We will then merge this PR with --no-ff to preserve PR number in git history. |
It looks so.
Ups sorry, I just merged, then read this. Shall I make a new branch and replay my commits there? |
The real
No longer relevant since I rolled back my
I had to stop them, but maybe I will run them again on some cold winter day. |
Oh, no, please don't bother, that's not a big deal. Also, if you rebase now, the merge commits are probably going to vanish (though you might have to solve a conflict or two manually, as is common with rebasing). Then just
Not really because, as I'm slowly learnings, ML is fine with lack of precision and sometimes it's even good (introduces a fuzz that helps break away from local minima and not to overfit) and definitely not worth a big performance price to eliminate. I think ML applications even prefer smaller floats than 64 bits in order to lower peak memory. [Edit: it's only serious numerical instability that becomes a problem and for which, indeed, exact arithmetic would be a solution.]
Great. Thanks.
I suggested in that ticket that running just the short test, the one that CI runs, may be a better idea than running both and concurrently. Did this suggestion make sense? |
Done.
I was expecting that. Now the question is, do we ever experience those serious errors. Anything else to do in this ticket? |
Let me check the comments... I think only this comment remains, if it makes sense to you (please ask if anything is unclear):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had a closer look at most of the formulas and they seem correct and probably fast, too. Thank you very much. Merging.
Fixes #19