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

Add rsqrt function and test cases #268

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

yashrajgupta
Copy link
Contributor

Adding rsqrt function from Recommended Elementary function.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 82.506% when pulling b6fcc80 on yashrajgupta:rsqrt_function_patch into 9a38d80 on JuliaIntervals:master.

@coveralls
Copy link

coveralls commented Mar 12, 2019

Coverage Status

Coverage increased (+0.03%) to 90.51% when pulling 5b302fb on yashrajgupta:rsqrt_function_patch into 1516c5e on JuliaIntervals:master.

@codecov-io
Copy link

codecov-io commented Mar 12, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@1516c5e). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master    #268   +/-   ##
========================================
  Coverage          ?   82.5%           
========================================
  Files             ?      25           
  Lines             ?    1229           
  Branches          ?       0           
========================================
  Hits              ?    1014           
  Misses            ?     215           
  Partials          ?       0
Impacted Files Coverage Δ
src/IntervalArithmetic.jl 50% <ø> (ø)
src/intervals/functions.jl 82.72% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1516c5e...5b302fb. Read the comment docs.

@dpsanders
Copy link
Member

I'm not sure that rsqrt is a good name for this function. Maybe invsqrt? In fact, is this function really necessary? I guess the idea is that it should be more accurate than inv(sqrt(X)).

@yashrajgupta
Copy link
Contributor Author

Actually the function is there with the same name in the document as well in octave. The implementation should give the tighest interval.

@lbenet
Copy link
Member

lbenet commented Apr 3, 2019

In the standard it appears as rSqrt; my guess is that "r" stands for "reciprocal". Note that the standard does not impose a specific name.

I agree with David that rsqrt is maybe not a good name. What about invsqrt, recipsqrt, reciprocalsqrt, or perhaps add some _ to make it more readable (e.g., reciprocal_sqrt).

Independently of the name, one more suggestion: add docstrings and also an entry into the documentation.

@yashrajgupta
Copy link
Contributor Author

Ok, I'll change the name and do the suggested.

@yashrajgupta yashrajgupta force-pushed the rsqrt_function_patch branch from 9ee216c to 0b45a07 Compare April 12, 2019 20:24
@lbenet
Copy link
Member

lbenet commented Apr 13, 2019

Any progress on this?

@yashrajgupta
Copy link
Contributor Author

Yeah I have changed the name and added a doc string. You can review it.

@dpsanders
Copy link
Member

Could you please rebase?

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