-
Notifications
You must be signed in to change notification settings - Fork 26
Compute coeffs using Rationals #65
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
Conversation
This looks like a fantastic improvement in accuracy -- I'm definitely in favour of doing this. Would definitely be good to prevent reverting by tying this down with a couple of unit tests. The above snippets would probably suffice. |
I think the improvements are only really big when using higher order and a high degree of |
Oh okay. Would be good to see some numbers on this then, both benchmarking and accuracy. |
tests have been added |
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.
LGTM modulo the typo! Thanks @oxinabox
e7e4750
to
7eed57f
Compare
Fixed #64
Turns out this can make a difference.
This not only gets result on julia 1.4 up to the standard of julia 1.2,
but actually does much more than that, significantly improving out accuracy
With this PR. in julia 1.2 or 1.4 (identical):
Without this PR. 1.4
Without this PR on 1.2
I feel like this is going to increase our cost a fair bit.
So it would be really good to get #61 done, to avoid recomputing coeffs.