You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
refactor(evaluation): shared groundwork for time-series metrics - #785
Move the grouping, column-selection, group-sampling and summary helpers
out of Autocorrelation Similarity into time_series_common, and register
time-series metrics in one TIME_SERIES_CARDS list that the multi-modal
report uses to run each component, build its figures and render its card.
- Report template loops over registered cards; card bodies use shared
comparison_notes and chart_grid macros with generic CSS and JS names.
- TimeSeriesValueColumnCheck validates value_columns for every
TimeSeriesMetricParameters field, so new metrics need no preflight code.
- Export autocorrelation_similarity_score to SafeSynthesizerSummary,
evaluation_metrics.json and W&B.
- Fix: automatic column selection follows training field types, so a
synthetic column that collapsed to a constant or a few integers is
scored instead of silently dropped. Columns synthetic data lacks or
stores as text are listed in the notes and details.
Signed-off-by: Sean Yang <seayang@nvidia.com>
Reviewing files that changed from the base of the PR and between d8e2de2 and d0a3e58.
Files selected for processing: 2
src/nemo_safe_synthesizer/TIMESERIES_README.md
tests/preflight/test_preflight.py
Files skipped from review as they are similar to previous changes: 2
tests/preflight/test_preflight.py
src/nemo_safe_synthesizer/TIMESERIES_README.md
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
Recent review details
Context from checks skipped due to timeout: 6
GitHub Check: CodeRabbit
GitHub Check: Unit Tests (3.12)
GitHub Check: Unit Tests (3.13)
GitHub Check: Unit Tests (3.11)
GitHub Check: Unit Tests (3.14)
GitHub Check: End-user Wheel Install
Walkthrough
This change adds shared configuration and evaluation helpers for time-series metrics. Autocorrelation Similarity uses the shared helpers and supplies report-card context. The multimodal report renders registered time-series cards, and the autocorrelation similarity score is included in summary and W&B output.
Adds a shared time-series metric configuration and helpers for selecting columns and groups, ordering sequences, and summarizing profiles. Preflight checks and tests cover configured value columns across metrics.
Autocorrelation evaluation and figures src/nemo_safe_synthesizer/evaluation/components/autocorrelation_similarity.py, src/nemo_safe_synthesizer/evaluation/components/autocorrelation_similarity_figures.py, tests/evaluation/components/test_autocorrelation_similarity.py
Autocorrelation Similarity delegates column and group handling and summary aggregation to shared helpers. It records excluded columns and supplies figure metadata for rendering. Tests cover collapsed values and columns excluded from comparison.
The report creates components from TIME_SERIES_COMPONENTS and renders cards from their contexts. Shared templates and time-series selectors handle chart content and display.
Score export and metric documentation src/nemo_safe_synthesizer/config/external_results.py, src/nemo_safe_synthesizer/results.py, tests/test_results.py, src/nemo_safe_synthesizer/TIMESERIES_README.md
Adds the autocorrelation similarity score to summary extraction and W&B output. The README describes metric behavior, report integration, and steps for adding a time-series metric.
NVIDIA-NeMo/Safe-Synthesizer#783: Adds a time-series metric that uses the configuration, preflight, report, and chart interfaces changed here.
Suggested labels: bug, feature, refactor
Suggested reviewers: binaryaaron
Merge Risk: Minimal · up to d0a3e
No identified issue blocks merging after normal checks.
Pre-merge checks: 5
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: shared evaluation infrastructure for time-series metrics.
Docstring Coverage
Docstring coverage is inconclusive: files from earlier reviews still need to be rechecked within the check's analysis limits. Not blocking.
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.
Finishing Touches
📝 Generate docstrings
Commit to this branch
Create a new PR
🧪 Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
seayang-nv
changed the title
refactor(evaluation): shared plumbing for time-series metrics
refactor(evaluation): shared groundwork for time-series metrics
Oct 21, 2026
The reason will be displayed to describe this comment to others. Learn more.
Shared methods and config parameters makes sense. But the time_series_cards.py feels like a weird extra abstraction layer. Is this a pattern we're thinking of using more and porting to the existing metrics? Could it be incorporated into the existing components classes instead of making a new setup? Though I get one is about computation and one is about rendering (sort of).
(In the future we'll probably relax the numeric only requirement on columns for some of these metrics, at a minimum autocorrelation I hope, but can handle that exception when/if it happens.)
Replace the TIME_SERIES_CARDS registry with a TimeSeriesComponent base
class. Each time-series component now carries its card attributes as
class variables and builds its figures in jinja_context, like the other
evaluation components, and the report runs a plain TIME_SERIES_COMPONENTS
tuple.
- Name left-out columns in the notes and details when no usable column or
group remains, not only on scored results.
- Drop function and class listings from module docstrings.
- Cover the remaining time_series_common guards.
Signed-off-by: Sean Yang <seayang@nvidia.com>
…value columns
Align TimeSeriesValueColumnCheck with the runtime reserved-column validation, and replace the outdated evaluation parameters section of TIMESERIES_README.md with the current nested configuration, marking time-series evaluation as in progress.
Signed-off-by: Sean Yang <seayang@nvidia.com>
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
Applicability reference: AGENTS.md: # AGENTS.md Durable implementation guidance belongs with the code it describes: function and class docstrings for public contracts and source comments for local invariants.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
github-actions
Bot
added
area:config
area:data-processing
area:evaluation
area:tests
area:docs
labels
seayang-nv
changed the title
coderabbitai
Bot
added
bug
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
kendrickb-nvidia
left a comment
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary
Moves the code every time-series metric needs into shared modules, so #780, #782 and #783 can each target
maininstead of stacking.time_series_common.py: grouping and column helpers, previously copied into every metric.TimeSeriesComponent: base class carrying each metric's card attributes; the report runs and renders the components listed inTIME_SERIES_COMPONENTS.time_series_charts.j2: shared card macros, with generic CSS and JS names.TimeSeriesValueColumnCheck: now checks every metric'svalue_columns, and rejects the timestamp and grouping columns.evaluation_metrics.jsonand W&B.Human review
Pre-Review Checklist
mise run formatleaves only intended formatting changes.mise run check ::: testpasses locally. Fails only onvllmimports, which aren't installed on macOS;mainfails the same way.mise run test:e2epasses locallymise run test:ci-containerpasses locally (recommended)/syncon this PR to trigger a run (auto-triggers on ready-for-review)Pre-Merge Checklist
Other Notes
Summary by CodeRabbit