-
Notifications
You must be signed in to change notification settings - Fork 140
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
Tune up sort #63
Tune up sort #63
Conversation
Earlier: Sort a Vector without any args 0.190000 0.000000 0.190000 ( 0.187377)
Sort vector in descending order with custom <=> operator 0.200000 0.000000 0.200000 ( 0.204298) Now: Sort a Vector without any args 0.070000 0.010000 0.080000 ( 0.074862)
Sort vector in descending order with custom <=> operator 0.120000 0.000000 0.120000 ( 0.123979) |
why aren't tests passing? |
Funny fail for Ruby 2.0 and 2.1 expected:
#<Daru::Vector:46315380 @name = nil @size = 6 >
nil
2 nil
4 nil
5 2
1 4
0 22
3 111
got:
#<Daru::Vector:46316220 @name = nil @size = 6 >
nil
4 nil
2 nil
5 2
1 4
0 22
3 111 |
LOL it's not sorting the nils appropriately. How about this fix: remove the nil elements, then sort the Array with |
@v0dro Are you talking about fixing this particular test or implementing this idea in |
} | ||
|
||
index_vector = @index.to_a.zip(@data) | ||
index_vector = index_vector.sort &block2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems no need to declare block2
as a separate variable. Simply pass { |a, b| block.call(a[1], b[1]) }
to #sort
.
Hmmm alright, just change the test. If the result is slightly different for different ruby versions don't forget to put the relevant comments in the code. |
Another option might be to sort on the index as a secondary sort key. That should make it consistent over different Ruby versions. Not sure about any performance impacts. |
@lokeshh could you try that out? Different indexes on different versions could prove to be problematic for certain use cases. |
Done. There's some negligible performance difference (increase of like 0.02 seconds). |
Set index as secondary key Improve performance Peformance Improvement
I am sorry, I just realized now that I forgot to place |
Create a new PR. |
Make sure you mention in the commit message that it's an amendment for this one. |
In reference to #39