thirtysixthspan / descriptive_statistics

MIT License
755 stars 69 forks source link

Overriding Array Sum method causes issues with active support's Enumerable Sum method. #29

Closed turlockmike closed 9 years ago

turlockmike commented 9 years ago

If an enumerable object contains a block or a any sort of non numerical values, this could cause unexpected behaviour.

Before Adding Gem {:test => 1}.sum{|k,v| "#{k} #{v}"} # "test 1"

After adding Gem {:test => 1}.sum{|k,v| "#{k} #{v}"} # [:test, 1]

Active Support is a very popular gem. It might be wise to implement it similar to active support. I can provide a patch if needed.

thirtysixthspan commented 9 years ago

Thanks! I added support for blocks https://github.com/thirtysixthspan/descriptive_statistics/commit/48d1701d851752da145e75c0d5a1dc3d40632077

However, your example will still not produce identical results for DescriptiveStatistics and ActiveSupport.

> require 'descriptive_statistics'
 => true
> {:test => 1}.sum{|(k,v)| "#{k} #{v}"}
 => 1.0

This is because DescriptiveStatistics pulls out hash values, discarding keys, for all operations.

> {a: 1, b: 2, c:3}.sum
 => 6.0

Furthermore, DescriptiveStatistics also converts all values to floating point values prior to any calculations. I'm not convinced that string concatenation should be the expected output of this library. I'm biased to limit the scope of functionality to valid numeric operations.

In your example, the output is 1.0 because the string in the block is converted into a floating point number. Note that the first value is 1 and the second is nil because of the way DescriptiveStatistics discards keys with hashes.

> "#{1} #{nil}".to_f
 => 1.0

Of course, I could be swayed and would love to see a PR, especially if it were to implement an ActiveSupport compatibility mode for DescriptiveStatistics.

Thanks again. I appreciate the feedback and the idea!