Skip to content

Preserve absent source counters as unknown measurements - #704

Closed
rudycelekli wants to merge 3 commits into
ktwu01:mainfrom
rudycelekli:fix/preserve-absent-source-metrics
Closed

rudycelekli wants to merge 3 commits into
ktwu01:mainfrom
rudycelekli:fix/preserve-absent-source-metrics

Conversation

@rudycelekli

@rudycelekli rudycelekli commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes #676 by distinguishing unknown counters from explicitly measured zero across all twelve affected connectors. The original patch is rebased onto current main and includes DataCite/OpenAIRE.

  • Reuse _reported_metrics to omit missing, null and empty counters while retaining measured zero and reported values.
  • Preserve unknown totals when GitHub release asset counts are incomplete.
  • Advance emitted connector parser versions to distinguish the changed interpretation.
  • Keep title-only historical migration pinned to its original /3 interpretation; new release fetches use /4.
  • Keep regression coverage in existing source/snapshot test modules, preserve the metrics schema and committed historical observations.

Native evidence

DataCite/OpenAIRE: eight failures plus four passing controls before the extension, followed by all 194 source tests passing. Twelve emitted-version checks fail on the earlier head and pass with the provenance update. The native title-only migration regression fails on 16e7afe and passes on 32e5e2d, retaining legacy stored metrics/hash and byte-identical second migration. The migration/source focused selection passes 195 tests.

Full clean verification

Exact signed head: 32e5e2d06627560df408042f4568ce855526353e.

All six required commands passed, in order, from a clean detached checkout with its own editable Python 3.12 environment, initialized pinned recursive submodule, and initially absent generated artifacts:

ruff check .
ruff format --check .
benchmark-radar normalize-catalog
benchmark-radar classify
benchmark-radar build-data-release
pytest -q

1,655 tests passed. Fresh generation preserves 3,212 records across five sources: Claire Radar 1,914; OpenCompass Hub 461; Artificial Analysis 25; LLM Stats 687; Model Reports 125. Index/shard keys and the archived payload agree. Earlier exact-head runs are recorded separately.

This change was prepared with AI assistance and reviewed against native reproduction and repository contribution rules. Hosted CI is tracked separately from local results.

@rudycelekli
rudycelekli requested a review from ktwu01 as a code owner September 29, 2026 22:57
@rudycelekli

Copy link
Copy Markdown
Contributor Author

Agent-authored PR annotation: 330226 GPT.

@toby-bridges toby-bridges left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

审查 head:97416a94cb7767530a282b63d968afec01358f15。

P2:合入当前 main 后,DataCite / OpenAIRE 仍把未报告计数写成零,#676 的全连接器修复还缺这两处。 这是集成后的覆盖缺口,不是声称本 PR 在原 head 新引入了这些旧行为。

动机

#676 要求区分“未报告”和“实测为零”,并明确提到 #613 / #614 应跟随统一规则。当前 PR 修复了旧基线上的十个连接器;今天 main 已包含 DataCite 和 OpenAIRE,需要一起核对。

改动思路

在现有 sources.py 中集中处理可选计数,再沿用 RadarItem.metrics 输出。单个计数键本就可缺省,无需新增 schema、依赖或第二套采集流程;也不重写历史快照。

具体改动

两个文件共 +106 / -40。生产代码覆盖 Hugging Face Hub、GitHub search / organizations、Kaggle、HF papers、Zenodo、Crossref、Semantic Scholar、GitHub Releases 和 OpenAlex;测试补充 HF、GitHub 和 release 的缺失计数案例。

关键代码讲解

  • _reported_metrics(sources.py:228):省略缺失、null 和空字符串,保留显式零及已有数值。
  • fetch_github_releases:只有资产列表存在且每项计数完整时才汇总下载量;显式空列表得到零,不完整列表不输出总量;仓库计数复用同一 helper。
  • fetch_huggingface:真实 fetch 入口验证了缺失 downloads 与显式 likes=0 可以并存。

对主干的风险

将 head 与 main 446f0e2b4dab6805c71af08a9c1a52ec511316c3 作本地临时合并,git merge-tree 无冲突。随后通过三个真实 fetch 函数执行同类上游 JSON 输入,仅替换网络响应:

输入 HF Hub DataCite / OpenAIRE
计数缺失 {} 各输出 citations/downloads/views 三个 0.0
显式零 保留零 保留零
已报告正值 保留原值 保留原值

这 9 个探针说明:干净合并仍留下六个 or 0 计数点。当前 main 的相关测试还把缺失等于零写成了预期。Issue 下已有贡献者提出后续协助,但当前集成结果尚不满足原 issue 的统一规则。

原 head 在干净 worktree、独立 venv、固定递归子模块中依次通过 ruff check .、ruff format --check .、benchmark-radar normalize-catalog、benchmark-radar classify、benchmark-radar build-data-release、pytest -q,1,402 passed。三个新增回归在旧基线均失败、在 head 均通过。环境为 macOS / Python 3.12;正式 CI 仍为 action_required。合并版本这里只运行了上述探针,没有宣称完整 CI 通过。

我的整体评价

现有 helper 和修复方向合适,没有复现其他阻塞问题。请更新到当前 main,把 DataCite 与 OpenAIRE 的六个计数也接入该 helper,并在它们现有测试中区分缺失、null、空字符串、零和正值,然后在新 head 重跑六步检查。无需新增测试框架。

English verdict: REQUEST_CHANGES - 97416a9; integrated current-main DataCite/OpenAIRE still turn unknown counters into zero. Head: 1402 tests passed; official CI pending.

Signed-off-by: Rudy Celekli <47457359+rudycelekli@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 6, 2026 09:33
@rudycelekli
rudycelekli force-pushed the fix/preserve-absent-source-metrics branch from 97416a9 to 575860e Compare October 6, 2026 09:33
@rudycelekli

Copy link
Copy Markdown
Contributor Author

Thanks for checking the current-main integration. I rebased the patch onto bcd699f05191ad0f97dd424280d9cf43da01745b and extended the existing _reported_metrics helper to all six DataCite/OpenAIRE counter sites. Unknown counters are omitted, while explicitly measured zero and positive values are retained. Historical snapshots are unchanged.

The new cases use the actual fetch_datacite and fetch_openaire entry points, replacing only the HTTP response. They cover missing keys, null, empty strings, zero, positive values, and mixed known/unknown counters in the existing tests/test_sources.py module. On the rebased patch before this extension, the cases produced 8 failures and 4 passing controls; afterward, all 194 source tests passed. The two sparse-record expectations now correctly expect omitted counters rather than fabricated zeros.

The signed new head 575860efbf16ef104cc364ff86adab514be561a6 passed all six required commands from a clean detached checkout with its own editable Python 3.12 environment, initialized pinned recursive submodule, and initially absent generated data: ruff check ., ruff format --check ., benchmark-radar normalize-catalog, benchmark-radar classify, benchmark-radar build-data-release, and pytest -q (1,655 passed).

The fresh release retains 3,212 records across all five sources, with index/shard keys and archived content checked for agreement. These are local verification results; hosted CI authorization/results are tracked separately.

@rudycelekli

Copy link
Copy Markdown
Contributor Author

330226 gpt-6.1-sol

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Connector parser versions must distinguish the changed metric interpretation, and two test comments contradict the new behavior.

Review effort: Balanced
Findings: 1 Medium severity · 1 Low severity

Open (2)
What changed in this PR

Preserves unknown source counters instead of coercing them to zero.

Changes:

  • Adds shared reported-metric normalization across connectors.
  • Handles incomplete GitHub release metrics.
  • Adds missing/null/zero counter regressions.
File Description
src/​benchmark_radar/​sources.py Preserves absent counters as unknown.
tests/​test_sources.py Adds connector regression coverage.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/benchmark_radar/sources.py
Comment thread tests/test_sources.py
Signed-off-by: Rudy Celekli <47457359+rudycelekli@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 6, 2026 09:58

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The shared GitHub release version also incorrectly relabels legacy title-migrated snapshots as using the new metric semantics.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Resolved since last review (2)

Comment thread src/benchmark_radar/sources.py
Signed-off-by: Rudy Celekli <47457359+rudycelekli@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 6, 2026 10:30

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The implementation consistently preserves unknown measurements, maintains historical provenance, and includes focused regression coverage.

Review effort: Balanced
Findings: None

Resolved since last review (1)

ktwu01 commented Oct 7, 2026

Copy link
Copy Markdown
Owner

Closing as not planned. One author now has a cap of 10 open issues + PRs, and this account had 56 open PRs and 11 open issues, mostly paired with each other. We merged the three changes that fix silent data loss in the corpus (#723, #774, and #773 once rebased) and are closing the rest so review stays possible.

This is not a verdict that the change is wrong. If you think this one matters, reopen it later when you are under the cap, one at a time, with a reproduction from a real user or caller rather than a hypothetical edge case.

不是否定这个改动本身。为保证评审能跟上,同一作者最多同时保留 10 个未关闭的 issue/PR;该账号曾有 56 个未关闭 PR 和 11 个 issue。已合并修复语料静默丢失的三个改动,其余先关闭。若认为本条重要,请在低于上限后逐个重新打开,并附真实用户或调用方的复现。


Generated by Claude Code

@ktwu01 ktwu01 closed this Oct 7, 2026
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.

Absent counters are published as zero in 19 places across ten connectors

4 participants