Repository navigation
feat(tools): dataset feature visualization tool - #3259
DoubleCore wants to merge 18 commits into
Conversation
Profile and visualize the feature characteristics of input vector datasets — per-dimension value distributions, vector norms, sparsity, duplicates, sampled pairwise distances, outliers, anisotropy, and an optional clustering summary — for the dense and sparse HDF5 formats used by pyvsag and the VSAG evaluation tooling. - deterministic seeded sampling with bounded memory (no full-dataset O(N^2) pairwise matrix); full vs sampled metrics are tagged with definition, scope, and sample size - versioned JSON report and a self-contained offline HTML visualization - optional provider-neutral AI interpretation layer: environment-variable credentials, aggregate-only payload (raw vectors are never sent), schema-validated responses with evidence metric keys, and graceful degradation when the provider is unavailable - unit and integration tests, reproducible toy HDF5 examples, and English/Chinese documentation Fixes: antgroup#2862 Signed-off-by: DoubleCore <851678303@qq.com> Assisted-by: OpenCode:qwen3.8-flash
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Reviewer | Review completeAutomated pull request review completed. Model: No actionable inline findings were found. |
Merger | Merge ReadinessHead: Updated: 2026-11-06 08:51:47 UTC Status: BLOCKED Merger decision: HOLD (shadow assessment; no merge executed).
Next owner: Author. Required next step: Inspect the required check failure and fix or verify its cause. Do not merge yet. Merge reliability: 0.00 / 1.00 (WITHHELD)
Evidence subtotal: 0.67; applicable ceiling: 0.00. Final score = min(subtotal, ceiling). Policy:
AI review: COMPLETE. Static review: patch is correct; CI is reported separately. Review findings: 0; P0/P1 findings in that review: 0; coverage: 34/34 changed files. Findings are not a count of currently unresolved threads. CI summary: 2 failure, 1 neutral, 14 skipped, 12 success. Check source: Next action: Merge commit freshness: Waiting for GitHub to refresh the merge commit for the current head and base. Only current head checks are shown.; PR CI Gate: failure; Human review: GitHub: REVIEW_REQUIRED; approvals >= 1; Code Owner approval required; stale approvals dismissed; Conversations: 86 unresolved; resolution required; GitHub merge state: BLOCKED CI check details
This is an uncalibrated evidence score, not a probability, correctness guarantee or approval. Zero means the assessment is withheld or lacks positive evidence, not a proven-bad patch. Automatic merging is disabled. New commits or base changes invalidate earlier evidence; never authorize a merge from this score alone. |
Merge Protections🟢 All 3 merge protections satisfied — ready to merge. Show 3 satisfied protections🟢 Require kind label
🟢 Require version label
🟢 Require linked issue for feature/bug PRs
|
vsag-bot
added
the
kind/feature
mergify
Bot
added
area/docs
|
Jev-driven classification Applied: Option probabilities
Model: |
vsag-bot
left a comment
vsag-bot
left a comment
There was a problem hiding this comment.
Reviewer | Code review
Automated pull request review completed.
Model: gpt-6.1-sol; reasoning effort: high.
Review scope: high (8360 changed lines across 34 files).
Submitted 8 inline comments.
Review coverage checks completed; this does not prove absence of defects.
Static review only; compilation and runtime tests are outside this review's scope.
File coverage: 34/34; observed passing test runs: 0.
Reviewed commit 6753077.
| method="POST", | ||
| ) | ||
| try: | ||
| with urllib.request.urlopen(request, timeout=self.timeout) as response: |
There was a problem hiding this comment.
[P1] [P1] Reject unsafe redirects before forwarding bearer credentials
If the configured HTTPS endpoint responds with a 302 redirect to HTTP or another host, urllib's default redirect handler follows it and copies the Authorization header. The initial HTTPS check does not protect this subsequent request, so the API key can be exposed in cleartext or sent to an unintended origin. Reject HTTPS downgrades and prevent cross-origin credential forwarding.
| } | ||
| metrics.update( | ||
| _sampled_metrics( | ||
| pair_matrix=pair_matrix, |
There was a problem hiding this comment.
[P2] [P2] Pass computed distances to the dense nearest-neighbor summary
Here pair_matrix is the M×D vector-coordinate array; the M×M result of _pairwise_dense was only used to obtain pair_values. _nearest_neighbor_metric consequently masks coordinate positions and takes coordinate minima. For [[0,0],[3,4],[6,8]], it summarizes [0,3,6] instead of Euclidean nearest-neighbor distances [5,5,5]. Retain and pass the computed distance matrix, as the sparse path does.
| # Invalid (NaN/Inf) entries are excluded from numeric summaries and treated | ||
| # as zero, matching the semantics of the norms metric. | ||
| sample_matrix = np.where(np.isfinite(sample_matrix), sample_matrix, 0.0) | ||
| pair_matrix = sample_matrix[:pair_n] |
There was a problem hiding this comment.
[P2] [P2] Select pairwise rows uniformly before taking a prefix
sample_indices returns indices sorted by dataset position, so this prefix selects the lowest-index sampled rows rather than a uniform subsample. With 1000 rows and default settings, the pairwise phase always uses rows 0–511 regardless of seed, excluding the remaining rows entirely. Ordered or clustered datasets therefore produce biased distance and near-duplicate summaries. Randomly select the bounded subset before sorting for reads; apply the same correction to sparse sample_records[:pair_n].
| return | ||
| index = ids[finite].astype(np.int64, copy=False) | ||
| vals = values[finite].astype(np.float64, copy=False) | ||
| np.add.at(self.count, index, 1.0) |
There was a problem hiding this comment.
[P2] [P2] Include implicit zeros in sparse dimension statistics
The accumulators count only explicitly stored finite entries, and arrays() passes them unchanged to _dimensions_metric. For two sparse vectors {0:2} and {}, dimension 0 is reported with mean 2, std 0, min 2, and zero ratio 0; the full-split values should be 1, 1, 0, and 0.5. Account for absent coordinates as zeros, while excluding explicitly invalid values, before calculating per-dimension summaries.
| return | ||
| new_size = max(size, max(16, self.count.size * 2)) | ||
| pad = new_size - self.count.size | ||
| self.count = np.concatenate([self.count, np.zeros(pad)]) |
There was a problem hiding this comment.
[P2] [P2] Bound sparse accumulators independently of feature ID magnitude
A valid sparse record containing feature ID 2^31 makes _ensure allocate six float64 arrays of length 2^31+1, roughly 96 GiB before temporary copies, even if the file contains only one stored value. The later dense-sample guard cannot prevent this allocation. Use sparse keyed accumulators or a checked fallback that preserves other metrics when the inferred dimension exceeds a safe memory budget.
There was a problem hiding this comment.
已在当前 head 确认修复：_profile_sparse 在推断维度超过 _MAX_SPARSE_STATS_DIMENSION 时仅停用 per-dimension 统计并保留其他指标，_DimAccumulator._ensure 也有独立检查，不会按 2^31 分配数组。7a76bf70 上重新运行 test_sparse_huge_feature_id_skips_only_dimension_arrays 与 test_sparse_accumulator_rejects_id_driven_oversized_allocation，均通过。
| method = "IQR fallback (MAD is zero)" | ||
| threshold = 3.0 * iqr | ||
| else: | ||
| mask = np.zeros(values.shape, dtype=bool) |
There was a problem hiding this comment.
[P2] [P2] Distinguish zero interquartile spread from identical norms
MAD and IQR can both be zero while extreme observations still exist. For 99 one-dimensional INT8 vectors with value 1 and one with value 100, this branch reports zero outliers and claims all norms are identical. Preserve detection of values outside the repeated central norm when both robust spread estimates vanish, instead of unconditionally clearing the mask.
There was a problem hiding this comment.
当前回退分支先用 np.all(values == median) 区分真正常量；MAD/IQR 都为零但存在偏差时使用 values != median，不再清零离群掩码。test_zero_mad_and_iqr_still_detect_extreme_norms 已在 7a76bf70 复测通过，99 个 1 加一个 100 会报告 1 个离群值。
| "pair_count": int(pair_values.size), | ||
| } | ||
| median = float(np.median(finite)) | ||
| threshold = max(0.0, median * float(epsilon)) |
There was a problem hiding this comment.
[P2] [P2] Avoid interpreting unnormalized IP scores as duplicate distances
The default sparse metric is IP, whose distance 1−dot can be negative and need not vanish for identical vectors. Thus [2,0] and [2,100] have distance −3 and are labeled near-duplicates despite being far apart, while two identical [0.5,0] vectors have distance 0.75 and fail the default threshold. Use a duplicate-compatible distance for this indicator, enforce the necessary normalization, or mark it unavailable for unnormalized IP.
There was a problem hiding this comment.
dense 与 sparse 的 IP profiling 均已将近重复检测切换为欧氏距离，并在报告中明确 metric = euclidean，不再把 1-dot 当作非负重复距离。test_ip_near_duplicates_use_nonnegative_duplicate_distance 和 test_sparse_ip_near_duplicates_use_euclidean_distance 已在 7a76bf70 复测通过。
| return None | ||
| if not math.isfinite(number): | ||
| return None | ||
| return round(number, 10) |
There was a problem hiding this comment.
[P2] [P2] Preserve small finite values during report serialization
Fixed rounding to ten decimal places turns every finite magnitude below about 5e-11 into zero. A valid FLOAT32 dataset with coordinates around 1e-12 consequently reports zero norms, means, and distances, and its distinct histogram edges collapse to zero. This affects JSON, HTML, and AI interpretation. Preserve floating-point values or round by significant digits rather than an absolute decimal scale.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6753077740
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| metrics.update( | ||
| _sampled_metrics( | ||
| pair_matrix=pair_matrix, |
There was a problem hiding this comment.
For dense inputs with at least two sampled vectors, pair_matrix is still the M×D feature matrix, but _sampled_metrics() forwards it to _nearest_neighbor_metric(), which expects an M×M distance matrix and takes row minima after masking the diagonal. Consequently, the nearest-neighbor statistics summarize feature coordinates rather than distances; the committed Euclidean example already reports a nearest-neighbor minimum of -858.9626464844. Retain the result of _pairwise_dense() and pass that distance matrix here.
Useful? React with 👍 / 👎.
| np.add.at(self.count, index, 1.0) | ||
| np.add.at(self.sum, index, vals) | ||
| np.add.at(self.sumsq, index, vals * vals) |
There was a problem hiding this comment.
For sparse inputs, coordinates absent from a record are zeros, but these accumulators count only explicitly stored finite values, and _dimensions_metric() divides by that occurrence count. A coordinate containing 10 in one of ten vectors therefore reports mean 10, standard deviation 0, and zero ratio 0, instead of 1, 3, and 0.9; minimum values and entirely absent coordinates are also wrong. Include implicit zeros when finalizing the per-dimension counts and aggregates, while continuing to exclude actual NaN/Inf entries.
Useful? React with 👍 / 👎.
| method="POST", | ||
| ) | ||
| try: | ||
| with urllib.request.urlopen(request, timeout=self.timeout) as response: |
There was a problem hiding this comment.
If the configured HTTPS endpoint returns a 301/302/303 redirect to HTTP or another origin, this default urlopen() call follows it with the bearer token intact. I verified that Python 3.14's HTTPRedirectHandler.redirect_request() preserves Authorization for both downgrade and cross-origin redirects. The constructor's HTTPS check therefore protects only the initial request, allowing a provider or reverse-proxy redirect to disclose the key despite the advertised HTTPS-only credential policy. Reject unsafe redirects before forwarding credentials, or disable automatic redirects.
Useful? React with 👍 / 👎.
| # Invalid (NaN/Inf) entries are excluded from numeric summaries and treated | ||
| # as zero, matching the semantics of the norms metric. | ||
| sample_matrix = np.where(np.isfinite(sample_matrix), sample_matrix, 0.0) | ||
| pair_matrix = sample_matrix[:pair_n] |
There was a problem hiding this comment.
When the pairwise limit is smaller than the main sample, as with the defaults, sample_indices() has already sorted the selected indices, so this prefix is biased toward earlier file rows rather than being a uniform subsample. For a 1024-row dataset, the default pairwise calculation uses rows 0–511 regardless of the seed. Files ordered by cluster or time consequently produce biased distance and near-duplicate estimates. Draw a seeded random subset of sample positions in both the dense and sparse paths instead of taking their ascending prefixes.
Useful? React with 👍 / 👎.
| mask = np.zeros(values.shape, dtype=bool) | ||
| method = "all norms identical; no outlier can be distinguished" | ||
| threshold = 0.0 |
There was a problem hiding this comment.
When most norms are identical but a few are extreme—for example, 99 norms of 1 and one of 1000—both MAD and IQR are zero even though the norms are not all identical. This branch then reports zero outliers and incorrectly labels the dataset as having identical norms, hiding the very norm spikes the metric is intended to detect. Distinguish genuinely constant data from a zero-spread majority and handle the remaining deviations explicitly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
该分支现已区分“所有 norm 相同”和“中心值重复但稳健离散度为零”；后者显式标记所有偏离 median 的值。test_zero_robust_spread_detects_deviations_on_both_sides 覆盖 99 个中心值及两端极端值，并已在 7a76bf70 复测通过。
vsag-lgtm
left a comment
vsag-lgtm
left a comment
There was a problem hiding this comment.
Automated review @ 67530777 — 3 inline suggestion(s).
| while True: | ||
| if offset + _LEN_SIZE > len(buffer): | ||
| break | ||
| (length,) = _LEN_STRUCT.unpack_from(buffer, offset) |
There was a problem hiding this comment.
[warning] Trusted record header length can force oversized allocations in the sparse byte-stream reader.
The no-offsets iterator only verifies that offset + record_size fits inside the current buffer before calling _decode_one_record. A malformed file can therefore declare a large length as long as the header-derived record_size is still within the chunk, and _decode_one_record will then attempt np.frombuffer(..., count=length) for ids and values before any further checks. This can allocate far more memory than the dataset actually contains. Consider capping length against the remaining bytes in the stream (e.g. len(buffer) - offset - _LEN_SIZE) before deriving record_size.
There was a problem hiding this comment.
no-offsets reader 现在会在等待完整 chunk 和调用 _decode_one_record 之前，将 header 推导的 record_size 与整个 byte stream 的实际剩余字节数比较；不可能的长度直接抛出 DatasetFormatError，不会进入 np.frombuffer 分配，同时仍支持合法记录跨 chunk。test_sparse_impossible_length_is_rejected_before_buffering 已在 7a76bf70 复测通过。
| position = 0 | ||
| while position < total_bytes: | ||
| end = min(position + max_chunk_bytes, total_bytes) | ||
| buffer += bytes(np.asarray(dset[position:end]).tobytes()) |
There was a problem hiding this comment.
[suggestion] Accumulating the entire sparse byte stream into a single bytes object raises peak memory for large files.
Every chunk is appended to buffer with buffer += bytes(...), which recreates the backing string on each iteration. When offsets are absent, the process can retain memory proportional to the full file size, undermining the streaming goal. Consider keeping a small residual window (for example with memoryview over the latest chunk plus any trailing partial record) instead of merging all previously consumed bytes.
| html_path = _default_output_path(args.input, "html") | ||
| else: | ||
| html_path = None | ||
| if html_path is not None: |
There was a problem hiding this comment.
[note] Explicit --out-json without --out-html silently disables HTML generation, which differs from the help text default.
The help string says --out-html defaults to <input>.profile.html, but this branch only falls back to that path when --out-json is also omitted. If this is intentional behavior, consider updating the help message so users do not expect an HTML report alongside a JSON-only request.
There was a problem hiding this comment.
--out-html 帮助文本现已明确说明：单独指定 --out-json 而未指定 --out-html 时会省略 HTML；只有两者都省略时才使用默认 HTML 路径。test_help_mentions_key_flags 已在 7a76bf70 复测通过。
…group#2862) Signed-off-by: DoubleCore <851678303@qq.com> Assisted-by: OpenCode:ark-code-latest
vsag-bot
left a comment
vsag-bot
left a comment
There was a problem hiding this comment.
Reviewer | Code review
Automated pull request review completed.
Review decision: Comment only. No approval was submitted.
Model: gpt-6.1-sol; reasoning effort: high.
Review scope: high (8644 changed lines across 34 files).
Submitted 9 inline comments.
Review coverage checks completed; this does not prove absence of defects.
Static review only; compilation and runtime tests are outside this review's scope.
File coverage: 34/34; observed passing test runs: 0.
Reviewed commit 96a2882.
| "split": report.get("input", {}).get("split"), | ||
| "dataset": report.get("dataset"), | ||
| "config": report.get("config"), | ||
| "metrics": report.get("metrics"), |
There was a problem hiding this comment.
[P2] [P2] Exclude per-vector projections from the aggregate AI payload
Forwarding all metrics includes clustering.projection_points and labels, even when clustering is disabled. For a one-dimensional split fully covered by the sample, the first PCA coordinate is each original value minus its mean, up to sign; the accompanying mean and min/max allow the original values to be recovered. Thus --ai sends recoverable per-vector data despite the documented aggregate-only boundary. Build an aggregate allowlist that omits per-vector projections and assignments.
| if metric == "euclidean": | ||
| squared_norms = np.einsum("ij,ij->i", matrix, matrix) | ||
| squared = ( | ||
| squared_norms[:, None] + squared_norms[None, :] - 2.0 * (matrix @ matrix.T) |
There was a problem hiding this comment.
[P2] [P2] Compute Euclidean distances without catastrophic cancellation
For valid FLOAT32 rows [[1e10, 0], [1e10, 1], [1e10, 2]], the norm and dot-product terms round to 1e20 in float64, so this subtraction returns zero for every pair instead of distances 1, 1, and 2. This corrupts pairwise and nearest-neighbor statistics and marks all pairs as near-duplicates. The sparse Euclidean branch has the same cancellation. Use bounded direct-difference calculations or a stable fallback for nearly cancelling terms.
| y_values = [value for _, value in pairs] | ||
| y_min, y_max = min(y_values), max(y_values) | ||
| if math.isclose(y_min, y_max): | ||
| y_min -= 1.0 |
There was a problem hiding this comment.
[P2] [P2] Use representable padding for large constant chart ranges
A valid FLOAT32 split [[1e20, 0], [0, 2e20], [2e20, 1e20]] has identical per-dimension means near 1e20. Adding or subtracting 1.0 at that magnitude leaves the float64 bounds unchanged, so sy() divides by zero. The default CLI consequently fails after writing JSON and never produces HTML. Expand constant ranges using scale-relative, representable padding.
| norms = np.linalg.norm(matrix, axis=1) | ||
| safe = np.where(norms > 0, norms, 1.0) | ||
| normalized = matrix / safe[:, None] | ||
| gram = normalized @ normalized.T |
There was a problem hiding this comment.
[P2] [P2] Avoid a full-sample matrix for the mean cosine summary
_anisotropy_metric passes the entire sample here, bypassing pairwise_sample_size. For example, --sample-size 100000 --pairwise-sample-size 512 on 16-D data reaches an 80 GB Gram allocation, followed by additional quadratic triangle arrays, although the configured distance matrix is only about 2 MB. Compute the mean from normalized-vector sums and diagonal contributions, or use bounded blocks, so increasing the reduction sample does not unexpectedly exhaust memory.
| vectors_with_invalid = 0 | ||
| all_zero_vectors = 0 | ||
|
|
||
| for _, block in dataset.iter_dense_blocks(block_rows=65536): |
There was a problem hiding this comment.
[P2] [P2] Bound dense blocks by their working byte size
The fixed 65536-row block makes streaming memory scale excessively with dimensionality. At 4096 FLOAT32 dimensions, one block alone retains 1 GiB of raw values, 2 GiB of converted values, and 256 MiB of finite flags before temporary arrays; subsequent iterations can retain previous working arrays during replacement. Small sampling settings cannot reduce this cost. Derive block_rows from a byte budget that accounts for the float64 working buffers.
| "full dataset", | ||
| ), | ||
| ("Element type", _e(str(dataset.get("dtype"))), "full dataset"), | ||
| ("Distance metric", _e(str(dataset.get("distance_metric") or "n/a")), "profiling metric"), |
There was a problem hiding this comment.
[P3] [P3] Display the effective profiling metric in the overview
dataset.distance_metric preserves the original file attribute, whereas config.metric holds the normalized metric actually used. Profiling a euclidean file with --metric angular therefore labels the overview's 'profiling metric' as euclidean while displaying angular-distance results; files using a fallback show n/a. Read the effective metric from report.config for this card.
| % (color, points) | ||
| ) | ||
| for index, value in pairs: | ||
| tip = "dimension %d: %.6g" % (index, value) |
There was a problem hiding this comment.
[P3] [P3] Label downsampled points with their source dimension ranges
These indices refer to bucket positions after stats.downsample, not original dimensions. For a 4-D dataset with --max-dimension-points 2, the two values average dimensions 0–1 and 2–3, but the tooltips identify them as dimensions 0 and 1 and the Coordinate axis ends at 1. Use source bucket ranges or centers for coordinates and tooltips so users can locate the dimensions represented by each plotted value.
| "<script>%s</script>\n" | ||
| "</body>\n</html>\n" | ||
| ) % ( | ||
| _e(dataset.get("file_name") or "dataset"), |
There was a problem hiding this comment.
[P3] [P3] Read the report filename from the input section
build_report stores file_name under report.input, and never puts it in report.dataset. Consequently every generated page title and header falls back to 'dataset', as the bundled HTML demonstrates, losing the input's identity when reports are compared or shared. Use report.input.file_name here and in _header.
| "count": 32, | ||
| "max": 2.3593201637, | ||
| "mean": -27.5290439874, | ||
| "min": -858.9626464844, |
There was a problem hiding this comment.
[P3] [P3] Regenerate the bundled reports with the current profiler
This Euclidean report contains a nearest-neighbor minimum of -858.96 and negative median, which the current non-negative Euclidean distance implementation cannot produce. The accompanying HTML repeats these values, and the sparse artifacts still omit implicit zeros and describe the old prefix sampling method. Regenerate all four bundled artifacts so the documented reproducible examples reflect the checked-in algorithm.
Distance metrics: compute dense Euclidean distances on directly shifted vectors when the Gram expansion is numerically unstable, and use direct differences for the sparse path, removing catastrophic cancellation that produced large negative nearest-neighbor distances. Memory: bound dense block working sets by a byte budget and chunk the mean pairwise cosine computation via the sum identity. AI payload: restrict aggregated metrics to an explicit allowlist so raw cluster labels and projection points are never uploaded. HTML report: derive the page title from the input file name, label charts with the source bucket boundaries, and only pad curves to representable values. Also regenerate the bundled example artifacts, sync the docs, and add regression tests for each fix. Signed-off-by: DoubleCore <851678303@qq.com> Assisted-by: OpenCode:deepseek-v4.1-flash
vsag-bot
left a comment
vsag-bot
left a comment
There was a problem hiding this comment.
Reviewer | Code review
Automated pull request review completed.
Review decision: Comment only. No approval was submitted.
Model: gpt-6.1-sol; reasoning effort: high.
Review scope: high (9050 changed lines across 34 files).
Submitted 4 inline comments.
Review coverage checks completed; this does not prove absence of defects.
Static review only; compilation and runtime tests are outside this review's scope.
File coverage: 34/34; observed passing test runs: 0.
Reviewed commit 6f71418.
| array = array[np.isfinite(array)] | ||
| if array.size == 0: | ||
| return {"bins": [], "counts": [], "bin_count": 0} | ||
| counts, edges = np.histogram(array, bins=int(bins)) |
There was a problem hiding this comment.
[P2] [P2] Handle large constant values before constructing histogram bins
A valid FLOAT32 dataset containing a single vector [260, 0] reaches this call with a constant norm of 260. NumPy's automatic constant-range expansion by ±0.5 rounds back to the same float64 value at that magnitude, so it cannot construct usable equal-width bins and raises. Because _norms_metric calls histogram unconditionally, profiling aborts before either report is produced. Use representable magnitude-aware padding or explicit edges for constant ranges.
| ) -> Dict[str, Any]: | ||
| with np.errstate(divide="ignore", invalid="ignore"): | ||
| mean = np.where(count > 0, total / count, np.nan) | ||
| variance = np.where(count > 0, total_sq / count - mean * mean, np.nan) |
There was a problem hiding this comment.
[P2] [P2] Accumulate dimension variance without subtracting large moments
The E[x²] - E[x]² formula can erase genuine variation even for supported FLOAT32 inputs. For one coordinate containing 255 copies of 230 and one value 230 + 128, both terms round to the same float64 number, yielding std=0 although the population variance is 63.75. This incorrectly presents a varying coordinate as constant in JSON, charts, and variance rankings. Use stable centered or online second-moment accumulation in both dense and sparse paths.
| def source_range(index: int): | ||
| if not downsampled: | ||
| return index, index | ||
| start = index * bucket_size |
There was a problem hiding this comment.
[P2] [P2] Reconstruct the actual downsampling boundaries
stats.downsample uses integer linspace boundaries, whose bucket widths differ when source_length is not divisible by the output length. Multiplying by the ceiling bucket_size does not recover those boundaries: with five dimensions and --max-dimension-points 2, values summarize dimensions 0–1 and 2–4, but this code labels them 0–2 and 3–4. Larger inputs can even receive nonexistent coordinate labels. Preserve exact boundaries or reconstruct the same linspace partition for positions and tooltips.
| matrix @ matrix.T | ||
| ) | ||
| scale = squared_norms[:, None] + squared_norms[None, :] | ||
| unstable = (scale > 0) & (squared <= _EUCLIDEAN_GRAM_REL_TOL * scale) |
There was a problem hiding this comment.
[P2] [P2] Exclude self-distances from the instability mask
Each ordinary nonzero row has a zero diagonal distance and positive diagonal scale, so its self-pair satisfies this instability test. Consequently every row enters the fallback loop even when all distinct pairs are well conditioned. A 512-row, 4096-dimensional sample allocates and scans roughly 8 GiB of difference buffers after already computing the Gram matrix; dense IP profiling also takes this path for duplicate detection. Clear the diagonal before selecting unstable rows so stable inputs retain the fast path.
There was a problem hiding this comment.
_dense_euclidean 现在在选择 unstable rows 之前执行 np.fill_diagonal(unstable, False)，自距离不会触发直接差值回退。test_dense_euclidean_skips_direct_fallback_for_self_distances 通过跟踪 np.einsum 确认稳定输入只执行初始 norm reduction，已在 7a76bf70 复测通过。
Signed-off-by: DoubleCore <doublecore@users.noreply.github.com> Assisted-by: OpenCode:ark-code-latest
|
修复已推送
|
…ples from current serializer Signed-off-by: DoubleCore <851678303@qq.com> Assisted-by: OpenCode:ark-code-latest
663e0f4 to
255bc8e
Compare
vsag-lgtm
left a comment
vsag-lgtm
left a comment
There was a problem hiding this comment.
Automated review @ 663e0f47 — 3 inline suggestion(s).
| """Estimate the bytes one sampled sparse row keeps after retention and | ||
| after the Python dict expansion performed by :func:`_pairwise_sparse`.""" | ||
| return ( | ||
| _SPARSE_SAMPLE_ROW_OVERHEAD_BYTES + int(ids.size) * _SPARSE_SAMPLE_ENTRY_BYTES |
There was a problem hiding this comment.
[warning] The 32-byte per-entry estimate under-counts the Python dict expansion that _pairwise_sparse performs, so this guard does not actually bound peak memory at _SAMPLE_MEMORY_BUDGET_BYTES (256 MiB).
_pairwise_sparse (profiler.py:1486) materialises {int(key): float(value) ...} for every sampled pair row, which costs roughly 104-108 bytes per entry on CPython (52 B dict slot + 28 B int + 24 B float); I measured ~108 B/entry on CPython 3.12. The retained ids/values arrays need only 8 B/entry, so of the 32 bytes charged here about 24 are left to pay for a ~108-byte expansion.
With the default budget the check admits ~8.4M retained entries. If pairwise_sample_size equals sample_size all of those rows expand into dicts (~900 MiB); even the default 512/1024 split still reaches ~450 MiB, and _sparse_sample_to_dense can add another 256 MiB on top. That is precisely the nnz-heavy case the guard is meant to keep in bounds.
Suggestion: raise _SPARSE_SAMPLE_ENTRY_BYTES (profiler.py:58) to cover the dict entry (>= 112) and/or charge the dense expansion against the same running total instead of a separate _SAMPLE_MEMORY_BUDGET_BYTES check in _sparse_sample_to_dense (profiler.py:1567).
| duplicate_metric, | ||
| int(pair_matrix.shape[0]), | ||
| duplicates_reason, | ||
| near_unavailable_reason=sample_budget_reason, |
There was a problem hiding this comment.
[suggestion] near_unavailable_reason makes the report contradict itself when both budgets trip at once.
When this budget stops sample retention and hashes is None (full-split duplicate state budget exceeded), _duplicates_metric still emits the note at profiler.py:1137-1141 — "... the near-duplicate indicator still covers the bounded pairwise sample" — while near_duplicate now carries available: false together with the budget reason passed in here. Every other consumer of that field is guarded, so please select the note based on whether near_unavailable_reason is set, and in that case state that both the exact and the near-duplicate indicators are unavailable.
| hashes[index] = np.frombuffer(_digest(payload), dtype=np.uint8) | ||
| if sample_budget_reason is None and index in sample_set: | ||
| row_bytes = _sparse_sample_row_bytes(ids) | ||
| if retained_sample_bytes + row_bytes > _SAMPLE_MEMORY_BUDGET_BYTES: |
There was a problem hiding this comment.
[suggestion] The report config still advertises the un-reduced sample size after this guard trips.
profile_dataset (profiler.py:185-210) only lowers the effective sizes through _dense_sample_size, so for a sparse dataset that trips this budget the config keeps e.g. effective_sample_size: 1024 and effective_pairwise_sample_size: 512 while all four sampled metrics and duplicates.near_duplicate report sample_size: 0 with available: false. The dense path instead reduces effective_sample and emits a dedicated warning. Recording the retained row count (in the config, or at least in the warning text) would keep the config and the metric entries consistent for report consumers.
… boundary checks Signed-off-by: DoubleCore <851678303@qq.com> Assisted-by: OpenCode:ark-code-latest
|
已在提交
本地验证： 即 148 passed / 0 failed，包含稀疏采样、超大 offset 正常路径及新增损坏边界回归测试。本地未编译 C++，仅运行了上述 Python 侧测试；此评论不表示 CI 已通过。 |
vsag-bot
left a comment
vsag-bot
left a comment
There was a problem hiding this comment.
Reviewer | Code review
Automated pull request review completed.
Review decision: Comment only. No approval was submitted.
Model: gpt-6.1-sol; reasoning effort: high.
Review scope: high (10448 changed lines across 34 files).
Submitted 3 inline comments.
Review coverage checks completed; this does not prove absence of defects.
Static review only; compilation and runtime tests are outside this review's scope.
File coverage: 34/34; observed passing test runs: 0.
Reviewed commit 9b4a59c.
| "schema_version": report.get("schema_version"), | ||
| "tool": report.get("tool"), | ||
| "split": report.get("input", {}).get("split"), | ||
| "dataset": report.get("dataset"), |
There was a problem hiding this comment.
[P2] [P2] Filter dataset metadata before sending it to the provider
This forwards dataset.datasets, which hdf5.py populates from every top-level HDF5 object name. Consequently, profiling train with --ai also transmits names of unrelated objects, including any customer identifiers or private labels embedded in those names. This exceeds the documented aggregate-only, bounded non-sensitive metadata boundary. Allowlist the necessary dataset properties and omit arbitrary object names.
There was a problem hiding this comment.
Fixed in 27be0fe. aggregate_payload now allowlists only vector type, dtype, count, dimensionality/inference, distance metric, and offsets presence. Arbitrary HDF5 object names (dataset.datasets) and the attributes map are no longer forwarded. Added a regression test with private-looking object/attribute values and verified neither appears in the provider prompt.
| effective_sample = _dense_sample_size( | ||
| dataset_count, int(metadata.dim or 0), requested_sample | ||
| ) | ||
| effective_pairwise = min(requested_pairwise, effective_sample) |
There was a problem hiding this comment.
[P3] [P3] Preserve actual sample counts after sparse budget exhaustion
When sparse sample retention exceeds its budget, _profile_sparse disables sampled work and supplies a 0-by-0 pairwise matrix, but this recomputation still advertises the requested dataset-limited counts. For example, 1024 sparse rows with 2048 entries each exceed the default retention budget, yet config and the HTML header report effective samples of 1024 and 512 although the sampled metrics report zero. Propagate the actual effective counts from the sparse profiling result.
There was a problem hiding this comment.
Fixed in 27be0fe. _profile_sparse now returns the actual effective sample and pairwise counts, and profile_dataset uses those values in the report config consumed by the HTML header. Budget exhaustion therefore reports 0/0 instead of the requested counts; the normal sparse path retains its actual counts. The sparse budget test now covers both cases and also ensures no misleading dense-budget warning is emitted.
| edges = edges[::-1] | ||
| counts, edges = np.histogram(array, bins=edges) | ||
| else: | ||
| counts, edges = np.histogram(array, bins=bin_count) |
There was a problem hiding this comment.
[P3] [P3] Handle narrow nonconstant histogram ranges
Valid FLOAT32 rows [1, 0] and [1, 2**-25] produce float64 norms 1 and 1 + 2**-51. These unequal values bypass the padding branch, so this call requests 20 bins across a range containing only two float64 ULPs. The edges necessarily repeat, and JSON/HTML expose zero-width intervals as separate histogram buckets. Apply representability padding to narrow nonconstant ranges too, or reduce the bin count.
There was a problem hiding this comment.
Fixed in 27be0fe. Narrow nonconstant ranges now reduce the histogram bin count until NumPy produces strictly increasing edges. This handles both NumPy behaviors: raising for too many finite-sized bins and returning repeated edges. Added the reported 1.0 / 1.0 + 2**-51 regression case and assertions that all intervals have positive width and both samples are retained.
vsag-lgtm
left a comment
vsag-lgtm
left a comment
There was a problem hiding this comment.
Automated review @ 9b4a59c0 — no inline suggestion(s); incremental changes since the previously reviewed commit are clean.
Signed-off-by: DoubleCore <851678303@qq.com> Assisted-by: OpenCode:ark-code-latest
|
Validation for 27be0fe: |
vsag-bot
left a comment
vsag-bot
left a comment
There was a problem hiding this comment.
Reviewer | Code review
Automated pull request review completed.
Review decision: Comment only. No approval was submitted.
Model: gpt-6.1-sol; reasoning effort: high.
Review scope: high (10518 changed lines across 34 files).
Submitted 1 inline comment.
Review coverage checks completed; this does not prove absence of defects.
Static review only; compilation and runtime tests are outside this review's scope.
File coverage: 34/34; observed passing test runs: 0.
Reviewed commit 27be0fe.
| distances[zero_norms, :] = 1.0 | ||
| distances[:, zero_norms] = 1.0 | ||
| distances[np.ix_(zero_norms, zero_norms)] = 0.0 | ||
| distances[np.abs(distances) <= _ANGULAR_ZERO_TOL] = 0.0 |
There was a problem hiding this comment.
[P2] [P2] Preserve small positive angular distances before thresholding
For FLOAT32 rows [1,0] and [1,2**-24], the angular distance is approximately 1.78e-15, but this clamp replaces it with zero because the tolerance is approximately 7.1e-15. With both rows sampled, the report therefore counts the pair as a near duplicate even when near_duplicate_relative_epsilon=0; the default relative threshold also incorrectly reports 100%. Recompute small distances from normalized coordinate differences or restrict zero canonicalization to equivalent directions. The sparse angular branch has the same issue.
There was a problem hiding this comment.
Fixed in a7d3805. The dense and sparse angular branches now recompute only near-zero Gram-based distances as half the squared normalized coordinate difference, so the reported FLOAT32 pair retains its positive 2**-49 distance instead of being clamped to zero; exactly identical normalized vectors still produce zero. Added dense/sparse regressions for both epsilon=0 and the default relative threshold. Validation: uv run --with pytest --with numpy --with scipy --with h5py python -m pytest tools/dataset_profiler -q (152 passed). No C++ was compiled locally; this Python-only change received static inspection plus Python tests.
vsag-bot
left a comment
vsag-bot
left a comment
There was a problem hiding this comment.
Reviewer | Code review
Automated pull request review completed.
Review decision: Comment only. No approval was submitted.
Model: gpt-6.1-sol; reasoning effort: high.
Review scope: high (10570 changed lines across 34 files).
Submitted 1 inline comment.
Review coverage checks completed; this does not prove absence of defects.
Static review only; compilation and runtime tests are outside this review's scope.
File coverage: 34/34; observed passing test runs: 0.
Reviewed commit a7d3805.
| for row in np.unique(np.nonzero(unstable)[0]): | ||
| columns = np.nonzero(unstable[row])[0] | ||
| differences = normalized[row] - normalized[columns] | ||
| direct = 0.5 * np.einsum("ij,ij->i", differences, differences) |
There was a problem hiding this comment.
[P2] [P2] Preserve zero angular distance for positive scalar multiples
For an angular dataset containing [[1,1],[3,3],[3,3]], independently rounded normalizations differ by one float64 ULP. This fallback assigns approximately 1.23e-32 to two mathematically zero distances. _near_duplicate_summary then multiplies that median by the default 1e-3, reporting only one near-duplicate pair instead of three. The sparse angular branch has the same issue. Handle normalization roundoff for exactly proportional vectors before applying the relative threshold, while preserving genuinely small positive angular distances.
There was a problem hiding this comment.
Fixed in 4a6ba2a. Both dense and sparse angular paths now canonicalize vectors by their largest absolute coordinate before computing norms. Exactly positive scalar multiples therefore have identical scaled coordinates and zero distance rather than normalization residue, while the direct-difference fallback still preserves genuinely small positive distances. Added dense/sparse regressions for [[1,1],[3,3],[3,3]], including all three near-duplicate pairs; the existing small-positive-distance regression also passes. Validation: uv run --with pytest --with numpy --with scipy --with h5py python -m pytest tools/dataset_profiler -q -> 154 passed. 本地未编译 C++，仅静态核对 + Python 测试；本次仅修改 Python。
Signed-off-by: DoubleCore <doublecore@users.noreply.github.com> Assisted-by: OpenCode:ark-code-latest
a7d3805 to
4a6ba2a
Compare
vsag-bot
left a comment
vsag-bot
left a comment
There was a problem hiding this comment.
Reviewer | Code review
Automated pull request review completed.
Review decision: Comment only. No approval was submitted.
Model: gpt-6.1-sol; reasoning effort: high.
Review scope: high (10606 changed lines across 34 files).
Submitted 2 inline comments.
Review coverage checks completed; this does not prove absence of defects.
Static review only; compilation and runtime tests are outside this review's scope.
File coverage: 34/34; observed passing test runs: 0.
Reviewed commit 4a6ba2a.
| limit = int(offsets[start]) + int(max_chunk_bytes) | ||
| end = int(np.searchsorted(offsets, limit, side="right")) - 1 | ||
| if end <= start: | ||
| end = start + 1 |
There was a problem hiding this comment.
[P2] [P2] Bound oversized sparse records before materializing them
When one record exceeds max_chunk_bytes, forcing end = start + 1 makes the following slice read the entire record anyway. A valid record with 100 million unique IDs occupies 800,000,004 bytes; decoding copies its arrays, and profiling allocates sorting and float64 buffers before checking the sample budget. This can exhaust several gigabytes despite the default 64 MiB chunk limit and sample_size=1. The sequential decoder likewise buffers a complete oversized record. Enforce a record working-memory limit before allocation, or process such records incrementally.
| ) | ||
|
|
||
| dset = f[self.split] | ||
| dtype = _describe_dtype(dset.dtype) |
There was a problem hiding this comment.
[P2] [P2] Reject non-byte storage for sparse splits
_describe_dtype accepts FLOAT32 for sparse inputs, although both sparse decoders assume shape[0] counts bytes. For example, type='sparse' with train=np.zeros(2, dtype=np.float32) and no offsets is accepted and reported as two empty vectors by reinterpreting the floats' eight bytes. Adding byte offsets instead fails validation because the sentinel is compared with the two-element shape. Require one-byte integer storage for sparse splits so unsupported layouts are rejected instead of producing offset-dependent or misleading profiles.
vsag-lgtm
left a comment
vsag-lgtm
left a comment
There was a problem hiding this comment.
Automated review @ 4a6ba2ac — 2 inline suggestion(s); no blocking issues found in this increment.
| digest_rows = np.ascontiguousarray(hashes).view( | ||
| np.dtype((np.void, _DUPLICATE_DIGEST_BYTES)) | ||
| ).reshape(-1) | ||
| _, group_sizes = np.unique(digest_rows, return_counts=True) |
There was a problem hiding this comment.
[suggestion] _full_split_state_reason admits this call when the digest buffer fits _FULL_SPLIT_STATE_BUDGET_BYTES at _DUPLICATE_BYTES_PER_VECTOR = 16 bytes per vector, but np.unique needs more than that input: a sorted copy of the digests (16 B/vector), the sort index working set (8 B/vector), the unique output (16 B/vector) and the counts (8 B/vector). Peak is therefore about 64 bytes per vector, roughly 3-4x the estimate the budget reason reports: at the boundary (16,777,216 vectors) the transient peak is close to 1 GiB while the message says 256 MiB. Consider raising _DUPLICATE_BYTES_PER_VECTOR to a peak-aware 64 so that both the gate and the reported estimate describe real usage.
| # is visible and renderable instead of exposing zero-width buckets. | ||
| while True: | ||
| try: | ||
| counts, edges = np.histogram(array, bins=bin_count) |
There was a problem hiding this comment.
[suggestion] Each retry re-scans the whole array, allocates a discarded array.size index array and throws the counts away; the loop can iterate up to bin_count times and is reached with full-split arrays (norms, sparse nnz_per_vector). Whether the automatic edges are distinct depends only on min/max, because np.histogram(..., bins=k) derives linspace(min, max, k + 1); the largest usable bin count can be found by probing those candidate edges in O(bin_count) work, then counting once with explicit bins=edges.
Signed-off-by: DoubleCore <doublecore@users.noreply.github.com> Assisted-by: OpenCode:ark-code-latest
|
Addressed both post-4a6ba2ac inline suggestions in e868d97, now pushed to this PR branch.
Local validation (not a CI result):
No C++ build was run for this Python-only dataset_profiler update. |
vsag-bot
left a comment
vsag-bot
left a comment
There was a problem hiding this comment.
Reviewer | Code review
Automated review finished with limited validation.
Review decision: Comment only. No approval was submitted.
Model: gpt-6.1-sol; reasoning effort: high.
Review scope: high (10625 changed lines across 34 files).
No publishable inline findings; validation remains incomplete.
Validation limited. This is not a clean bill of health.
Static review only; compilation and runtime tests are outside this review's scope.
File coverage: 34/34; observed passing test runs: 0.
- One or more planned validation steps failed or were not run.
Reviewed commit e868d97.
vsag-lgtm
left a comment
vsag-lgtm
left a comment
There was a problem hiding this comment.
Automated review @ e868d97f - 1 inline suggestion(s).
| # np.unique additionally holds a sorted copy, sort index, unique output, and | ||
| # counts, bringing peak duplicate-detection state to about 64 bytes per vector. | ||
| _DUPLICATE_DIGEST_BYTES = 16 | ||
| _DUPLICATE_BYTES_PER_VECTOR = 4 * _DUPLICATE_DIGEST_BYTES |
There was a problem hiding this comment.
[suggestion] Sync the user-facing profiler docs with the new duplicate-detection budget accounting.
This change budgets exact-duplicate detection at 64 bytes per vector (the retained 16-byte digest row plus np.unique's sorted copy, sort index, unique output, and counts), so duplicates.exact now silently becomes unavailable above roughly 4.19M vectors instead of ~16.7M - a user-visible reporting change. tools/dataset_profiler/TECHNICAL_REPORT.md was updated for this, but the user-facing pages were not: docs/docs/en/src/resources/dataset_profiler.md line 75 and docs/docs/zh/src/resources/dataset_profiler.md line 67 still say the exact-duplicate digests are "16 bytes per vector" with "a 256 MiB retained-state budget", which no longer matches the accounting introduced at this line. Please update both localized pages to document the 64-byte peak-aware accounting and the resulting availability threshold.
Signed-off-by: DoubleCore <doublecore@users.noreply.github.com> Assisted-by: OpenCode:ark-code-latest
|
Addressed in
Validation: |
vsag-lgtm
left a comment
vsag-lgtm
left a comment
There was a problem hiding this comment.
Automated review @ 40c880ab — docs-only increment (en/zh dataset_profiler.md); verified the new exact-duplicate budget wording against the implementation (_DUPLICATE_BYTES_PER_VECTOR = 64, shared 256 MiB full-split budget, 4,194,304-vector threshold, norms/nnz at 8 bytes per vector) — consistent, no inline findings.
vsag-bot
left a comment
vsag-bot
left a comment
There was a problem hiding this comment.
Reviewer | Code review
Automated pull request review completed.
Review decision: Approved. No actionable findings were identified in this complete static review. CI and merge requirements remain separate.
Model: gpt-6.1-sol; reasoning effort: high.
Review scope: high (10632 changed lines across 34 files).
No actionable inline findings were found.
Review coverage checks completed; this does not prove absence of defects.
Static review only; compilation and runtime tests are outside this review's scope.
File coverage: 34/34; observed passing test runs: 0.
Reviewed commit 40c880a.
Implements #2862. Dataset feature visualization tool for pyvsag.
Fixes: #2862