Repository navigation
GenderExtractor: compute the second-highest pronoun count order-independently - #859
ManasBagul23 wants to merge 1 commit into
Conversation
…endently
The max/second-max tracking loop only updated secondCount when the
running max was dethroned by a later, larger count:
if (count > maxCount) {
secondCount = maxCount.toDouble
maxCount = count
maxGender = gender
}
So secondCount stays at its initial 0.0 whenever the eventual overall
max happens to be the first entry Scala's Map iteration produces for
this page's pronoun counts, even if a genuinely close second-place
count exists. The confidence check maxCount/secondCount > minDifference
then divides by 0.0, evaluates to Infinity, and always passes, exactly
when it should be guarding against a near-tie.
Confirmed with the same counts in both iteration orders: male=10
processed first gives secondCount=0.0 (ratio Infinity, always
"confident"), female=9 processed first gives the correct secondCount=9.0
(ratio 1.11). Same data, different outcome, purely from unspecified map
iteration order.
Sort the counts descending and read off the top two instead, so the
result no longer depends on iteration order.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly and concisely describes the main change: computing the second-highest pronoun count independently of iteration order. |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Docstring Coverage
-
Explanation
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches: 💡 1
-
🧪 Generate unit tests (beta)
-
- Create a new PR
🛠️ Fix failing CI checks: 💡
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
The max/second-max tracking loop in
GenderExtractoronly updatessecondCountwhen the runningmax is dethroned by a later, larger count:
So
secondCountstays at its initial0.0whenever the eventual overall max happens to be thefirst entry Scala's
Mapiteration produces for a page's pronoun counts — even when a genuinelyclose second-place count exists. The confidence check
maxCount/secondCount > minDifferencethendivides by
0.0, evaluates toInfinity, and always passes, exactly when it should be guardingagainst a near-tie.
Confirmed with the same counts in both iteration orders:
male=10processed first givessecondCount=0.0(ratioInfinity, always "confident");female=9processed first gives thecorrect
secondCount=9.0(ratio1.11). Same data, different outcome, purely from unspecifiedmap iteration order — this is a real, data-dependent nondeterminism, not a hypothetical.
Fix: sort the counts descending and read off the top two, so the result no longer depends on
iteration order.
Summary by CodeRabbit