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

Adds functions to calculate the midpoint_radius representation. #35

Merged
merged 1 commit into from
Jun 22, 2015

Conversation

dpsanders
Copy link
Member

Adds a version of find_roots that returns a tuple of the midpoints, a tuple of the radii, and a tuple of the symbols

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.52%) to 73.76% when pulling 3f0326b on midpoint_radius into 86b9f53 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.29%) to 73.76% when pulling 9471568 on midpoint_radius into ae4e84a on master.

@dpsanders dpsanders mentioned this pull request May 23, 2015
13 tasks
@coveralls
Copy link

Coverage Status

Coverage decreased (-5.25%) to 71.8% when pulling 580cc02 on midpoint_radius into ae4e84a on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-5.93%) to 71.12% when pulling fc1affa on midpoint_radius into ae4e84a on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-5.93%) to 71.12% when pulling fc1affa on midpoint_radius into ae4e84a on master.

@dpsanders
Copy link
Member Author

Well let's just leave it as find_roots_midpoint for the moment and think about a better interface.
This is ready to be merged; the red X comes from coveralls and not from Travis. (I.e., we need more tests...)

Added inverse trig functions

Added tests for inverse trig functions

Added floor and ceil for intervals

Added tests for asin and acos when interval is not in domain. Added inverse trig functions in automatic_differentiation (need tests)

Remove Float from newton tests due to Travis

Added midpoint_radius

Removed clean_roots and its necessity; added find_roots_midpoint

Added with_interval_precision

Restore sort! for roots in newton and krawczyk

Generate Wilkinson polynomials using Horner's method

Minor changes

Added possibility to have array references of form a[i] inside @interval

Rewrite symbol generation for W3 since previous version only worked on Julia v0.4

Separate out Wilkinson polynomials into separate file. Use lexicographic ordering for sorting roots

Reduced some keyword argument complexity in Newton and Krawczyk

Make code slightly more readable

Changed Root to be a true immutable type. Fixes for various versions of newton, find_roots etc. Tests pass

Periodic points notebook
@dpsanders
Copy link
Member Author

Squashed. Ready to merge I think.

@lbenet
Copy link
Member

lbenet commented Jun 22, 2015

@dpsanders I really like lots of the changes and additions.

One suggestion: Change the output of midpoint_radius. Currently we get:

julia> @interval 0.1
[0.09999999999999999, 0.1]

julia> midpoint_radius(ans)
(0.1,1.3877787807814457e-17)

I think it looks nicer that the output of the last line is something like:

0.1 ± 1.3877787807814457e-17

@lbenet
Copy link
Member

lbenet commented Jun 22, 2015

Along the same lines:

julia> set_interval_precision(BigFloat, 53)
53

julia> @interval 0.1
[9.9999999999999992e-02, 1.0000000000000001e-01]₅₃

julia> midpoint_radius(ans)
(1.0000000000000001e-01,1.3877787807814457e-17)

The last line doesn't show that it is a BigFloat interval of 53 bits of precision...

@@ -1,39 +1,127 @@

# immutable Root{T<:Real}
Copy link
Member

Choose a reason for hiding this comment

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

I really like the idea; have you benchmark it against previous behavior? I would guess it is much faster now, isn't it?

Copy link
Member

Choose a reason for hiding this comment

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

This was meant on line 14...

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't benchmarked it. The change was necessary to have more readable code (and results). I expect it doesn't change the speed too much (at least on v0.4).

@lbenet
Copy link
Member

lbenet commented Jun 22, 2015

I'll wait for your reply about my comment. I agree it is ready to be merged.

@dpsanders
Copy link
Member Author

Your two comments about the display are good ones, but not relevant for this PR:

  1. The first comment, about output like 3 ± 0.1, seems to me to be really about how intervals should be displayed. We could add a global option to change this output for displaying any intervals.

The function midpoint_radius is designed only to calculate and return the values for later use; changing its output would require actually making a new type with its own show, but really it's just a different representation of the same interval, so this doesn't seem like a good idea.

I have opened issue #39 for this, but I think we can leave this til after 0.1.

  1. The issue about how BigFloats are displayed is due to the recent change Display BigFloat's based on their precision JuliaLang/julia#11578 in base Julia; in v0.3 the output is
(1.0000000000000001e-01 with 53 bits of precision,5.551115123125783e-18 with 53 bits of precision)

We can fix this in #39 for intervals; this example with 53 bits suggests that the idea of just removing the indication of the precision in the output of BigFloats was not necessarily a good idea...!

@dpsanders
Copy link
Member Author

The red dot is due to the coverage decrease; tests pass on Travis.
We definitely need better test coverage, but merging for the moment.

dpsanders added a commit that referenced this pull request Jun 22, 2015
Adds functions to calculate the midpoint_radius representation.
@dpsanders dpsanders merged commit b3b2f12 into master Jun 22, 2015
@dpsanders dpsanders deleted the midpoint_radius branch June 22, 2015 13:23
@lbenet
Copy link
Member

lbenet commented Jun 22, 2015

I agree with both comments, and that this is merged.

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.

None yet

3 participants