SciRuby/daru

Vector to vector binary operations cannot be done if the indexes contain both strings and symbols

thomasnaude opened this issue · 5 comments

thomasnaude commented 
/Users/tom/.rbenv/versions/2.4.3/lib/ruby/gems/2.4.0/gems/daru-0.2.1/lib/daru/maths/arithmetic/vector.rb:95:in `sort': comparison of Symbol with String failed (ArgumentError)
from /Users/tom/.rbenv/versions/2.4.3/lib/ruby/gems/2.4.0/gems/daru-0.2.1/lib/daru/maths/arithmetic/vector.rb:95:in `v2v_binary'
from /Users/tom/.rbenv/versions/2.4.3/lib/ruby/gems/2.4.0/gems/daru-0.2.1/lib/daru/maths/arithmetic/vector.rb:82:in `binary_op'
from /Users/tom/.rbenv/versions/2.4.3/lib/ruby/gems/2.4.0/gems/daru-0.2.1/lib/daru/maths/arithmetic/vector.rb:18:in `/'

to reproduce :

first_vector = Daru::Vector.new([1, 2, 3], index: [:one, 'two', 'three'])
second_vector = Daru::Vector.new([1, 2, 3], index: [:one, 'two', 'three'])

first_vector / second_vector
Shekharrajak commented

Thanks @thomasnaude for pointing it out this issue. I think, to resolve this issue one has to look into, why sorting is done in binary operation of vectors here .

thomasnaude commented

You are right @Shekharrajak, that is definitely the problem. I looked into it and could not figure out why the sorting is done. But that certainly does not mean it is not needed.....

cyrillefr commented

Hello, @Shekharrajak , I looked at the code.
In order to keep the sorting, we could add a simple:sort_by(&:to_s).
By doing that, we ensure a sorting by string, and so the call succeed.
I did a little test ans it works fine.

Shekharrajak commented

Sorry for late response. That's great @cyrillefr ,PR is welcome.

cyrillefr commented

Hello @Shekharrajak, @thomasnaude

Sorry for the delay.

I did not test the whole suite of specs and the simple trick I mentioned leads to fail another test.
That one : it_should_behave_like correct macd in spec/maths/statistics/vector_spec.rb @ line 735

The macd method (in lib/daru/maths/statistics/vector) that in turns tries to: add ema(fast) - ema(slow)
So method v2v_binary operation, other, opts={} is called and index is sorted.
BUT, if sorted like that(sort_by(&:to_s)), it is not equal to the numeric sort, and (I don't know why) in turns,
there are problems in the ema method at this line:
start = @data.index { |i| !i.nil? }

Why ?
Because with a sort(numeric in the test), all the nils are padded to right, and start is well defined.
BUT, with a to_s sort, nils are spread everywhere in the array and a nil error is raised.
There is a #FIXME line in the v2v_binary method about "why the sort?"
I think I maybe have found out why the sort.

Nevertheless, I needed a sort_by(&:to_s), but ONLY when needed, so I had the idea to add a utility method, that
would first try to sort, and only upon fail would string-sort.
And, all the tests pass(including one previously pending).

  • 👍1