Skip to content
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

Quartic formula, no obvious bugs #187

Merged
merged 4 commits into from
May 19, 2014
Merged

Quartic formula, no obvious bugs #187

merged 4 commits into from
May 19, 2014

Conversation

Mathnerd314
Copy link
Contributor

This should work. Tell me if I did anything wrong.

@fryguybob
Copy link
Member

It would be nice to look into some numerical methods works and see if there is a comparison between using an explicit quartic formula and some other iterative algorithm. I suspect that the quartic formula will have cases with significant inaccuracy. I'm not saying not to include this, but it would also be nice to have appropriate iterative root finding methods and have a handle on when to use which. Also I agree with @jeffreyrosenbluth about style. Finally, the toler tolerance should be an explicit argument with a default argument baked into another function. I could be hardcoded if you had a proof that it is the best possible tolerance for the particular Floating instance.

@bergey
Copy link
Member

bergey commented May 18, 2014

First of all, I think we're all being more nitpicky than usual for a commit this size. @Mathnerd314, it's because we're looking forward to lots more code from you that we want to talk about these style points immediately. (I'm very glad that folks are reading and commenting.)

I really like the comment density. I always find this kind of numeric code hard to follow, but I think the comments make it about as coherent as possible. If you can take out a layer or two of nesting, as @jeffreyrosenbluth suggests, or assign names to some of the intermediate results, that will help even more.

Otherwise, I agree with what others' have said. quartForm' and cubForm' for the versions taking an extra tolerance argument would be consistent with other names in Diagrams.

@bergey
Copy link
Member

bergey commented May 18, 2014

Oh, and I think someone should open a new ticket about some numerical root-finding method. It's probably not a high priority since we don't yet have any code that uses polynomials above cubic.

@Mathnerd314
Copy link
Contributor Author

I filed #188.

--
-----------------------------------------------------------------------------
module Diagrams.Solve
( quadForm
, cubForm
, quartForm
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should export cubForm' and quartForm' also. Otherwise, looks good to me.

@byorgey
Copy link
Member

byorgey commented May 19, 2014

Looks good to me.

bergey added a commit that referenced this pull request May 19, 2014
Add function to find roots of quartic polynomials
@bergey bergey merged commit a5ca24b into master May 19, 2014
@bergey bergey deleted the quartic branch May 19, 2014 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants