Pull requests

#8 Declined
Repository
yasirs yasirs
Branch
default
Repository
pypy pypy
Branch
default

added numpy.sort, tests, and docstring for numpy.array.sort

Author
  1. yasirs
Reviewers
Description
No description
  • Learn about pull requests

Comments (8)

    1. yasirs author

      Sure, do you want to let me know the concern so I can try to resolve it? (whether it is about conforming to cpython's numpy, or testing, performance, or architecture etc.)

      Thanks!

      ps. I sent this earlier from my inbox without realising that that was a private communication. Apologies if a double copy of this message arrives.

      1. Alex Gaynor

        Sorry, here are the issues I saw:

        • It would need to be rewritten after the merge of the dtypes branch (which rewrites most of everything :P)
        • It doesn't work with any of the virtual arrays (I'm surprised it even translates)
        • It looks like a pretty simple quicksort, which means it can have performance issues (I'd like to try using the TimSort we already have)
        • There are stylistic issues, semicolons, and excessive parens mostly.
        1. yasirs author

          I think the major issue is that of dtypes. I had imagined a number of passes to add other sorting algorithms and improvements through writing tests. I agree that it doesn't work for virtual arrays. (I think you mean stuff like slices, right?). I guess my brief question is, if people are interested in contributing to numpy/scipy development, are there specific helpful things to do? Perhaps a to do list with tests or a roadmap? Or just wait for the dtypes branch to be merged to the default and then go from there?

    2. Armin Rigo

      Oups, sorry about merging too eagerly the previous pull request.

      I can foresee a time when these pull requests will become annoying... yasirs should just be given access rights to the main repo, and just work in a branch.

    1. Pypy repo owner

      I don't think this is complete. The algorithm looks a bit to simple and also it can just reuse timsort from rlib with a little work. Also, as for a sorting algo reimplementation it definitely lacks tests (there is no odd-sized list, no empty list for one)

      Cheers, fijal

    2. yasirs author

      I abandoned it assuming these features should be added after merging the dtypes branch after reading Alex's comments. I'd still like to work on numpy development though.