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

Topic/sorting 721 #738

Merged
merged 12 commits into from
Aug 27, 2018
Merged

Topic/sorting 721 #738

merged 12 commits into from
Aug 27, 2018

Conversation

ghostroad
Copy link
Contributor

@ghostroad ghostroad commented Aug 21, 2018

This is intended to resolve Issue #721, which relates to inconsistencies across the implementations of MergeSort, InsertionSort, and QuickSort.

  1. All these strategies operate in place on segments of arrays that start and end at specified indices. In the QuickSort implementation, the end index was inclusive, while it was exclusive in MergeSort and InsertionSort. This has been fixed in this changeset to make the end index exclusive in all three implementations, following the convention in the Java Collections library. Validity and boundary checks have been introduced for the various parameters. Unit tests have been written to document and test the new behavior.

  2. The naming of the start and end indices has been made consistent.

  3. Unit tests have been added for checking correctness, stability, edge cases, etc.

  4. Scaladocs have been added for all the utility methods in the Sorting package.

@codecov-io
Copy link

codecov-io commented Aug 21, 2018

Codecov Report

Merging #738 into master will increase coverage by 0.11%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #738      +/-   ##
=========================================
+ Coverage   51.28%   51.4%   +0.11%     
=========================================
  Files         196     196              
  Lines       11765   11774       +9     
  Branches     1615    1591      -24     
=========================================
+ Hits         6034    6052      +18     
+ Misses       5731    5722       -9
Impacted Files Coverage Δ
core/src/main/scala/spire/math/Sorting.scala 100% <100%> (+46.42%) ⬆️
...ore/src/main/scala/spire/math/interval/Bound.scala 48.81% <0%> (-5.52%) ⬇️
...ala/spire/math/poly/BigDecimalRootRefinement.scala 82.11% <0%> (-3.32%) ⬇️
core/src/main/scala/spire/math/Natural.scala 65.75% <0%> (-1.82%) ⬇️
core/src/main/scala/spire/math/package.scala 59.1% <0%> (-0.35%) ⬇️
core/src/main/scala/spire/math/Algebraic.scala 65.85% <0%> (ø) ⬆️
core/src/main/scala/spire/math/Interval.scala 67.86% <0%> (ø) ⬆️
core/src/main/scala/spire/math/Rational.scala 95.52% <0%> (+0.23%) ⬆️
extras/src/main/scala/spire/math/FixedPoint.scala 34.97% <0%> (+0.54%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93d3b4d...8716d67. Read the comment docs.

@denisrosset
Copy link
Collaborator

denisrosset commented Aug 27, 2018

Beautiful PR! Thanks for the hard work; I've incorporated some of your notes in the future release notes.

Love the complete tests in particular.

@denisrosset
Copy link
Collaborator

Hi! If you'd like to see your real name in the CHANGES.md file, please make a PR towards it, I'd be happy to merge.

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.

3 participants