Match Math.sqrt to default behaviour with Integers #43

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants

I realise that the mathn library changes this, but I think most developers rely on Math.sqrt returning a float like the default math.rb library. As such when we use ruby-unit + other gems that rely on Math.sqrt returning a Float we can get issues (alchemist, or rgeo for example).

Aaron Todd Match Math.sqrt to default behaviour with Integers
I realise that the mathn library changes this, but I think most developers rely on Math.sqrt returning a float like the default math.rb library. As such when we use ruby-unit + other gems that rely on Math.sqrt returning a Float we can get issues (alchemist, or rgeo for example).
ca34f40
Owner

olbrich commented May 23, 2012

Unit math is done using rationals as much as possible to avoid problems with round off errors. It also uses mathn internally. To my knowledge, the behavior you are seeking to enforce is not documented nor guaranteed to be the case for the standard Math.sqrt function.

Can you provide me with a more detailed example of how this causes problems?

Hi there, I just loaded up the ruby core documentation and it shows that the return from Math.sqrt should in fact be a float :

http://www.ruby-doc.org/core-1.9.3/Math.html#method-c-sqrt

sqrt(numeric) → float

I understand that you're using Mathn and so this overrides this behaviour to return Complex and Rational figures as appropriate, however for integers this then means that it can also return an Integer :

Using Mathn : Math.sqrt(1) == 1
Using Math : Math.sqrt(1) == 1.0

In our case where we are using the RGeo gem, it relies on the sqrt(1) being 1.0 (a float) but instead due to the overrides present in ruby-units, sqrt(1) is 1 (an Int).

I believe that the typical output from Math.sqrt is probably expected to conform to the standard Math library behaviour, hence the request to have the fix applied inside of ruby-units.

Let me know if the example above isn't making sense :)

Cheers,
Aaron Todd

On 23/05/2012, at 9:45 PM, Kevin Olbrich wrote:

Unit math is done using rationals as much as possible to avoid problems with round off errors. It also uses mathn internally. To my knowledge, the behavior you are seeking to enforce is not documented nor guaranteed to be the case for the standard Math.sqrt function.

Can you provide me with a more detailed example of how this causes problems?


Reply to this email directly or view it on GitHub:
#43 (comment)

kranzky commented Jul 19, 2012

I think we can agree that mathn (which is a Ruby core library) can cause bugs in 3rd-party gems that may make assumptions about Math (such as the assumption in RGeo that you should be able to call nan? on the result of Math.sqrt, which breaks when it starts returning Integers).

Another problem with mathn is that it's not very performant. For example:

#!/usr/bin/env ruby

def test_performance
  require 'benchmark'
  puts Benchmark.realtime { 1_000_000.times { 1 / Math.sqrt(2) } }
end

test_performance

require 'ruby-units'

test_performance

On my machine, this prints:

1.55383014678955
15.2412781715393

That is, using ruby-units has caused my benchmark to take almost 10 times as long to run. And the benchmark isn't even doing any unit calculations at all!

I believe a fundamental criteria of good library design is that there shouldn't be any side-effects of this kind. Using ruby-units shouldn't cause other gems to break. And I believe that the choice of greater numerical accuracy should be in the hands of the user of the gem (that is, we should be able to choose whether we'd prefer performance or accuracy).

My suggestion is that ruby-units should use mathn if it is already in the namespace, rather than requiring it internally. Users of ruby-units that prefer the accuracy that comes with mathn will need to require mathn explicitly. Other users, such as us, who prefer performance over accuracy and who need to fix unintended side-effects such as what we're getting with Rgeo, will simply not require mathn.

kranzky commented Nov 28, 2013

Worth closing this; the issue will not be resolved within ruby-units.

olbrich closed this Nov 8, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment