-
Notifications
You must be signed in to change notification settings - Fork 0
docs: make issue and PR references readable instead of dead links #9
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -28,11 +28,16 @@ No live correctness bug exists in any realistic, supported code path. The core M | |||||||||||
|
|
||||||||||||
| ## Resolution status | ||||||||||||
|
|
||||||||||||
| Added 2026-08-18. **All 22 findings were addressed in v0.3.1 ([#59](https://github.com/ArcInstitute/gpudge_arc/pull/59))**, the release immediately following this review; two of them deliberately by deciding no code should change. Statuses below were re-verified against the tree at `v0.8.0`, and the *Evidence* column names something you can open — a regression test that cites the finding ID, or the code that now carries the guard. | ||||||||||||
| > `gpudge_arc#59` and similar tokens below are **provenance, not links**: they | ||||||||||||
| > identify entries in an issue tracker that is not public and will not resolve | ||||||||||||
| > from here. They are kept so each disposition can be traced to the change that | ||||||||||||
| > made it. | ||||||||||||
|
Comment on lines
+31
to
+34
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To maintain grammatical consistency with the note added to
Suggested change
|
||||||||||||
|
|
||||||||||||
| Added 2026-08-18. **All 22 findings were addressed in v0.3.1 (`gpudge_arc#59`)**, the release immediately following this review; two of them deliberately by deciding no code should change. Statuses below were re-verified against the tree at `v0.8.0`, and the *Evidence* column names something you can open — a regression test that cites the finding ID, or the code that now carries the guard. | ||||||||||||
|
|
||||||||||||
| | # | Severity | Finding | Status | Evidence | | ||||||||||||
| |---|---|---|---|---| | ||||||||||||
| | H1 | High | NaN/None `groupby` labels bucketed into `'nan'`/`'None'` groups | **Fixed** (v0.3.1); guard broadened in v0.8.0 ([#124](https://github.com/ArcInstitute/gpudge_arc/pull/124)) | `_ingest.MISSING_LABEL_SPELLINGS` and the mirroring guard in `_shard_stream.py`, which was the only streaming layout when this was fixed; `_cell_stream.py` gained the same screen when the cell layout arrived in v0.7.0. ⚠️ One backend-parity gap remains open as [#127](https://github.com/ArcInstitute/gpudge_arc/issues/127) — a group *genuinely* named `nan` is rejected on streaming but accepted in memory | | ||||||||||||
| | H1 | High | NaN/None `groupby` labels bucketed into `'nan'`/`'None'` groups | **Fixed** (v0.3.1); guard broadened in v0.8.0 (`gpudge_arc#124`) | `_ingest.MISSING_LABEL_SPELLINGS` and the mirroring guard in `_shard_stream.py`, which was the only streaming layout when this was fixed; `_cell_stream.py` gained the same screen when the cell layout arrived in v0.7.0. ⚠️ One backend-parity gap remains open as `gpudge_arc#127` — a group *genuinely* named `nan` is rejected on streaming but accepted in memory | | ||||||||||||
| | M1 | Medium | MWU-vs-scipy correctness tests all `@needs_cuda` | **Fixed** | `test_mwu.py::test_mwu_ref_matches_scipy_cpu`, which cites M1 | | ||||||||||||
| | M2 | Medium | Streaming equivalence checked Pearson correlation only | **Fixed** | `test_shard_stream.py::_assert_equiv` — `allclose(rtol=1e-5, atol=1e-7, equal_nan=True)` over full row coverage, with the M2 reasoning at the call site | | ||||||||||||
| | M3 | Medium | `environment.yml` pinned `gpudge @v0.2.0` | **Fixed** | pins the current release tag; kept in step with README by a note in both | | ||||||||||||
|
|
@@ -51,7 +56,7 @@ Added 2026-08-18. **All 22 findings were addressed in v0.3.1 ([#59](https://gith | |||||||||||
| | N2 | Nit | `.item()` forces per-chunk GPU→CPU syncs | **No change** | a perf nit with no correctness impact; the syncs are still there in `_mwu.py` | | ||||||||||||
| | N3 | Nit | A group literally named `'all_others'` is remapped | **No change, deliberate** | this review's own recommendation was "no action before the legacy spelling is removed"; the behaviour is noted in `__init__.py` | | ||||||||||||
| | N4 | Nit | `mwu_ref` sentinel test checked only the p-value half | **Fixed** | `test_mwu.py` now asserts `(U[ref_idx] == 0).all()`, citing N4 | | ||||||||||||
| | N5 | Nit | `MeanCalc` / `__version__` absent from the README API docs | **Fixed** | both documented in the README ([#126](https://github.com/ArcInstitute/gpudge_arc/pull/126)) | | ||||||||||||
| | N5 | Nit | `MeanCalc` / `__version__` absent from the README API docs | **Fixed** | both documented in the README (`gpudge_arc#126`) | | ||||||||||||
| | N6 | Nit | `csr_row_sums` docstring said "CSR" but accepts dense | **Fixed** | now "Per-row sum of a CSR sparse OR dense matrix" | | ||||||||||||
| | N7 | Nit | README/SKILL listed 5 of the 10 default output columns | **Fixed** | both now list all ten under a `columns (10):` heading | | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -9,6 +9,10 @@ defect from a live one without cross-referencing the changelog. A findings list | |||||||||
| without dispositions reads as a list of known, open bugs — the opposite of what | ||||||||||
| publishing it is for. | ||||||||||
|
|
||||||||||
| Issue and PR numbers on this page and in the reports — `gpudge_arc#59` and | ||||||||||
| similar — are provenance: they identify entries in an issue tracker that is not | ||||||||||
| public and will not resolve from here. | ||||||||||
|
Comment on lines
+12
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For visual consistency with the notes added in
Comment on lines
+12
to
+14
Comment on lines
+12
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To maintain consistency with the phrasing used in
Suggested change
|
||||||||||
|
|
||||||||||
| | report | findings | disposition | | ||||||||||
| |---|---|---| | ||||||||||
| | [2026-06-13](2026-06-13-gpudge-ultrareview.md) | 23 survived verification, of 28 raw | status table in the report: 9 confirmed defects, 5 fixed in the same PR, 4 filed as issues | | ||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For phrasing consistency with the note in
CHANGELOG.md("They are kept so that..."), consider using "so that" here as well: