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

Tune up sort #63

Merged
merged 1 commit into from
Mar 3, 2016
Merged

Tune up sort #63

merged 1 commit into from
Mar 3, 2016

Conversation

lokeshh
Copy link
Member

@lokeshh lokeshh commented Feb 29, 2016

In reference to #39

@lokeshh
Copy link
Member Author

lokeshh commented Feb 29, 2016

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)

@v0dro
Copy link
Member

v0dro commented Feb 29, 2016

why aren't tests passing?

@lokeshh
Copy link
Member Author

lokeshh commented Feb 29, 2016

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

@v0dro
Copy link
Member

v0dro commented Mar 1, 2016

LOL it's not sorting the nils appropriately.

How about this fix: remove the nil elements, then sort the Array with Array#sort, and then prepend nils at the beginning of the vector. The nil indexes can be in any order. @gnilrets what do you think?

@lokeshh
Copy link
Member Author

lokeshh commented Mar 1, 2016

@v0dro Are you talking about fixing this particular test or implementing this idea in vector#sort?

}

index_vector = @index.to_a.zip(@data)
index_vector = index_vector.sort &block2
Copy link
Member

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.

@v0dro
Copy link
Member

v0dro commented Mar 1, 2016

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.

@lokeshh lokeshh changed the title [WIP]Tune up sort Tune up sort Mar 1, 2016
@gnilrets
Copy link
Contributor

gnilrets commented Mar 3, 2016

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.

@v0dro
Copy link
Member

v0dro commented Mar 3, 2016

@lokeshh could you try that out? Different indexes on different versions could prove to be problematic for certain use cases.

@lokeshh
Copy link
Member Author

lokeshh commented Mar 3, 2016

Done. There's some negligible performance difference (increase of like 0.02 seconds).

Set index as secondary key

Improve performance

Peformance Improvement
v0dro added a commit that referenced this pull request Mar 3, 2016
@v0dro v0dro merged commit fdfee89 into SciRuby:master Mar 3, 2016
@lokeshh
Copy link
Member Author

lokeshh commented Mar 4, 2016

I am sorry, I just realized now that I forgot to place nils at the top when order is descending. What to do now? Should I just push the new commit in this PR or should I create another PR?

@v0dro
Copy link
Member

v0dro commented Mar 4, 2016

Create a new PR.

@v0dro
Copy link
Member

v0dro commented Mar 4, 2016

Make sure you mention in the commit message that it's an amendment for this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants