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

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

Open
thomasnaude opened this issue Mar 21, 2019 · 5 comments

Comments

@thomasnaude
Copy link

/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
Copy link
Member

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Member

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

cyrillefr added a commit to cyrillefr/daru that referenced this issue Nov 2, 2020
 - added a utility method to fall back to string-sort
   in case sort raise error
 - this metod called in v2v_binary method
 - impending test removed and passes
@cyrillefr
Copy link
Contributor

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).

thomasnaude pushed a commit to thomasnaude/daru that referenced this issue Aug 17, 2022
thomasnaude added a commit to thomasnaude/daru that referenced this issue Aug 17, 2022
…r-binary-operations

Issue with v2v bin operation and indexes (SciRuby#501)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants