SciRuby / daru

Data Analysis in RUby
BSD 2-Clause "Simplified" License
1.03k stars 139 forks source link

Fixes min, max, index_of_max and index_of_min for Daru::Vector #387

Closed athityakumar closed 6 years ago

lokeshh commented 6 years ago

@athityakumar Why are you sorting to find the min/max? Sorting takes O(n log n) while min/max can be done in O(n).

athityakumar commented 6 years ago

@lokeshh - Array#max_by takes argument of n for number of max_by elements to return, from Ruby 2.2.0 onwards and is NOT present in Ruby 2.1.0. Hence, sorting is a workaround to have Daru::Vector#max_by(n) work even for Ruby 2.1.0 environments.

I should probably add a note regarding this, so that these methods don't get forgotten to be changed while deprecating support for Ruby 2.1.0.

v0dro commented 6 years ago

Can you add a statement that will run a particular algorithm if RUBY_VERSION is greater than 2.1.0? That way there's no need of the note, we'll find out and remove it anyway.

parthm commented 6 years ago

Should we be using Gem::Version for comparison? Gem::Version.new('0.4.1') > Gem::Version.new('0.10.1').

athityakumar commented 6 years ago

@v0dro - Acknowledged, I've added the check for RUBY_VERSION < 2.2.0. However, Travis CI is now failing as coverage in statistics/vector.rb is less than 95% for Ruby 2.2+ versions.

v0dro commented 6 years ago

Well then how about adding the coverage and fixing the issue? :P

v0dro commented 6 years ago

Very weird failure on ruby 2.0 and 2.1.

athityakumar commented 6 years ago

@v0dro @zverok - Ruby 2.2+ now have a coverage of 94.53% ( < 95%, duh). Also, I'm seeing some pretty strange failure on Ruby 2.0. I tried (hard) reverting to a previous commit whose Travis CI build was successful, and it turns out to fail now. And now I think it's because of backport's removal by PR #394.

2.0.0-p648 :001 > [[1,2],[3,4]].to_h #! Typical failures
NoMethodError: undefined method `to_h' for [[1, 2], [3, 4]]:Array
2.0.0-p648 :002 > require 'backports' #! This has now been removed from daru
 => true
2.0.0-p648 :003 > [[1,2],[3,4]].to_h #! No failure
 => {1=>2, 3=>4}

Is it time to drop support for Ruby 2.0 as per issue #336?

baarkerlounger commented 6 years ago

@athityakumar it's the removal of backports completely in @zverok's PR. I fixed it here https://github.com/SciRuby/daru/pull/398 by just requiring the specific backport needed.

zverok commented 6 years ago

Yes, it was a misunderstanding during solving backports problem. I believe we'll solve it just by merging #398, without separate PR :)

zverok commented 6 years ago

Oh, to hell with that, quickfixed the CI problem myself :) @athityakumar please rebase from master, everything should be OK.

athityakumar commented 6 years ago

@v0dro @zverok - Finally managed to get the coverage issue solved. 😄

v0dro commented 6 years ago

@athityakumar ping!

athityakumar commented 6 years ago

@v0dro @zverok - Added the changes suggested above, and got caught again in the vicious cycle of coverage < 95%. 😢

zverok commented 6 years ago

OK, I am officially done with that "coverage" limitation, I'll remove it in master shortly. We need another way for enforcing good coverage.