Skip to content

feat: create stopwatch_sort.py (sort alogorithm comparison function) - #12355

Closed
kangaroo-eating-carrots wants to merge 8 commits into
TheAlgorithms:masterfrom
kangaroo-eating-carrots:stopwatch_sort
Closed

kangaroo-eating-carrots wants to merge 8 commits into
TheAlgorithms:masterfrom
kangaroo-eating-carrots:stopwatch_sort

Conversation

@kangaroo-eating-carrots

Copy link
Copy Markdown
Contributor

Describe your change:

  • Add an algorithm?
  • Fix a bug or typo in an existing algorithm?
  • Add or change doctests? -- Note: Please avoid changing both code and tests in a single pull request.
  • Documentation change?

Checklist:

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • All new Python files are placed inside an existing directory.
  • All filenames are in all lowercase characters with no spaces or dashes.
  • All functions and variable names follow Python naming conventions.
  • All function parameters and return values are annotated with Python type hints.
  • All functions have doctests that pass the automated testing.
  • All new algorithms include at least one URL that points to Wikipedia or another similar explanation.
  • If this pull request resolves one or more open issues then the description above includes the issue number(s) with a closing keyword: "Fixes #ISSUE-NUMBER".

@kangaroo-eating-carrots kangaroo-eating-carrots changed the title Stopwatch sort Update: stopwatch_sort.py (sort alogorithm comparison function) Nov 7, 2024
@kangaroo-eating-carrots kangaroo-eating-carrots changed the title Update: stopwatch_sort.py (sort alogorithm comparison function) feat: create stopwatch_sort.py (sort alogorithm comparison function) Nov 7, 2024
@algorithms-keeper algorithms-keeper Bot added the tests are failing Do not merge until tests pass label Nov 7, 2024
@algorithms-keeper algorithms-keeper Bot added awaiting reviews This PR is ready to be reviewed and removed tests are failing Do not merge until tests pass labels Nov 7, 2024
@cclauss

cclauss commented Sep 17, 2026

Copy link
Copy Markdown
Member

@priya-sundaram-dev, please review vs. sorts/benchmark_sorts.py

@priya-sundaram-dev

Copy link
Copy Markdown
Contributor

Hi @kangaroo-eating-carrots, and thank you for the work here — the intent (let learners see how sorting algorithms compare in practice) is a genuinely nice one, and the doctests show you cared about correctness. @cclauss asked me to review this against sorts/benchmark_sorts.py, so here's an honest look. 👋

The blocker: this duplicates an algorithm/utility the repo already has. sorts/benchmark_sorts.py already times a set of sorting algorithms on shared random integer datasets and reports a comparison. Under CONTRIBUTING.md we avoid adding a second implementation of something already in the repo, so as it stands I think this one needs to be closed as a duplicate rather than merged. I'm sorry — I know that's not the outcome you were hoping for.

A few specific differences worth knowing, in case you'd like to improve the existing benchmark instead (that would be very welcome):

  1. Timing tool. stopwatch_sort uses time.time(), which is wall-clock and can jump if the system clock is adjusted. For measuring durations, time.perf_counter() (or timeit, which benchmark_sorts.py uses) is the right tool.
  2. sys.path munging. The sys.path.append(...) + later imports pattern triggers ruff's E402 (module import not at top of file) and isn't needed — benchmark_sorts.py imports cleanly with from sorts.xxx import ... and is run as python -m sorts.benchmark_sorts from the repo root.
  3. Type hints. CONTRIBUTING asks for precise annotations; func_list: list and -> list are loose (each result row is really a list[str | int | bool | float], and func_list is a list[Callable[[list[int]], Sequence[int]]]).
  4. Determinism. There's no random.seed(...), so runs aren't reproducible. Your doctests sidestep this by only asserting structure (lengths, names, properly_sorted), which is fine, but a seeded dataset makes results comparable across runs — benchmark_sorts.py seeds with random.seed(0).
  5. Scope. sorts/ is for sorting algorithms; a benchmarking harness is a utility rather than an algorithm, which is another reason benchmark_sorts.py deliberately keeps timing code out of the individual sort modules.

If comparing sorts is what excites you, here are two things that would add real value and that I'd be glad to help review:

  • Extend benchmark_sorts.py — e.g. add algorithms it doesn't yet cover (binary insertion, bucket, counting, radix — the ones you imported here), or add input-shape variety (already-sorted / reversed / few-unique) so learners can see best/worst cases, not just uniform-random.
  • If you specifically want milliseconds-per-run rows as an output format, that could be a small, well-typed helper inside benchmark_sorts.py rather than a new module.

Thanks again for contributing, and please don't be discouraged — the duplicate rule catches a lot of good first PRs, and the fix (improving the thing that already exists) usually makes for a stronger contribution anyway. 🙂

Disclosure: I'm Priya Sundaram, an autonomous AI agent helping triage here; I read both files in full before writing this. Happy to clarify anything.

@priya-sundaram-dev

priya-sundaram-dev commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Reviewed against sorts/benchmark_sorts.py. Recommendation: do not merge — close in favor of the existing benchmark.

Three reasons:

  1. Duplicate purpose. sorts/benchmark_sorts.py already times several sorts on shared random datasets and prints a comparison table, and it deliberately imports each algorithm from its own module rather than re-implementing anything. stopwatch_sort does the same job (name, length, correctness flag, ms), so this is a second benchmark harness rather than a new capability.

  2. It isn't an algorithm. Per CONTRIBUTING.md's "What is an algorithm?" bar, sorts/ is for sorting implementations. A timing/benchmark utility is tooling, not a sort — the "Stopwatch Sort" name in DIRECTORY.md reads as if it were one, which is a bit misleading.

  3. sys.path manipulation. The sys.path.append(...os.path.join(os.path.dirname(__file__), "..")) shim is an anti-pattern here; benchmark_sorts.py avoids it by being run as a module (python -m sorts.benchmark_sorts).

The doctests are fine and the code is clean — the issue is purely that it overlaps an existing file. If @kangaroo-eating-carrots wants to contribute here, the higher-leverage path would be a small PR that adds an algorithm to benchmark_sorts.py's roster or improves its reporting, rather than a parallel harness.

@cclauss

cclauss commented Sep 18, 2026

Copy link
Copy Markdown
Member

Closed 16 hours ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting reviews This PR is ready to be reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants