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

Sorting package / convention for (start, end) and (left, right) #721

Closed
3 tasks
denisrosset opened this issue Apr 9, 2018 · 6 comments
Closed
3 tasks

Comments

@denisrosset
Copy link
Collaborator

denisrosset commented Apr 9, 2018

The convention is not consistent across sort methods: sometimes the end/right point is inclusive, sometimes not.

This is in https://github.com/non/spire/blob/master/core/src/main/scala/spire/math/Sorting.scala

Suggested steps:

  • Document the existing convention
  • Verify that we test for the boundary conditions. If not, write additional tests
  • Harmonize the convention across methods
@stephen-lazaro
Copy link

If you all are interested in help with this, I'm happy to crack at it this weekend assuming no one beats me to it.

@denisrosset
Copy link
Collaborator Author

Yes, please do!

When harmonizing, I suggest reusing the Java convention where end is not inclusive.

Anyway, it is not possible to allocate arrays bigger than INT_MAX - 2 or so.

@denisrosset
Copy link
Collaborator Author

@stephen-lazaro Let me know if you need on that issue!

@stephen-lazaro
Copy link

Thanks! Sorry there hasn't been much movement on this, got caught up in work a bit more than I expected.

@ghostroad
Copy link
Contributor

@stephen-lazaro @denisrosset To become more familiar with Spire, I took a crack at this. I just submitted the pull request.

Mainly I added some unit tests so as to make tweaking the implementation safer. As per the suggestion, the indices are now consistent in their meaning across the methods (end is always exclusive).

@denisrosset
Copy link
Collaborator Author

Solved by #738 ! Thanks

JoeWrightss pushed a commit to JoeWrightss/spire that referenced this issue Apr 13, 2019
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