Skip to content

Increase default NPS for the Sphere and the Sphere SDDR#516

Open
mcampos16 wants to merge 1 commit into
developingfrom
increase_Sphere+SDDR_NPS
Open

Increase default NPS for the Sphere and the Sphere SDDR#516
mcampos16 wants to merge 1 commit into
developingfrom
increase_Sphere+SDDR_NPS

Conversation

@mcampos16
Copy link
Copy Markdown
Contributor

@mcampos16 mcampos16 commented Apr 29, 2026

This pull request applies an increase in the default NPS of the Sphere and the Sphere SDDR benchmarks from 1E8 to 1E9.
This change is applied to the run_cfg.yml file.

After the comparison between OpenMC (run with 1E8 particles) and MCNP (run with 1E9 particles) Sphere results, the benefit of applying this modification was highlighted. For example, for subcase Sphere_68164_Er-164:
image

Summary by CodeRabbit

  • Chores
    • Updated benchmark configuration parameters to enhance testing consistency across specified configurations.

@mcampos16 mcampos16 requested a review from dodu94 April 29, 2026 15:21
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 29, 2026

Walkthrough

The PR updates the nps parameter in the benchmark configuration file from 1e8 to 1e9 for two experiment entries: Sphere and SphereSDDR, increasing the number of sample points used in these benchmark tests.

Changes

Cohort / File(s) Summary
Benchmark Configuration
src/jade/resources/default_cfg/run_cfg.yml
Updated nps parameter values from 1e8 to 1e9 for Sphere and SphereSDDR experiment entries.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

Suggested reviewers

  • dodu94

Poem

🐰 A hop, a skip, and values grow,
From one-oh-eight to nines we go,
Sphere benchmarks now shall sample more,
Each test refined, precision's core! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and accurately describes the main change: increasing the NPS parameter from 1E8 to 1E9 for the Sphere and Sphere SDDR benchmarks in run_cfg.yml.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch increase_Sphere+SDDR_NPS

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.

❤️ Share
Review rate limit: 0/1 reviews remaining, refill in 60 minutes.

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/jade/resources/default_cfg/run_cfg.yml (1)

189-209: ⚠️ Potential issue | 🟡 Minor

Duplicate benchmark entries with inconsistent configurations.

The configuration contains duplicate entries that should be resolved:

  1. IPPE-DT appears twice (lines 189-198 and 310-319) with identical configurations
  2. IPPE-CF appears twice (lines 200-209 and 321-330) with different NPS values (1e9 vs 1e8)

The IPPE-CF duplication with conflicting NPS values is particularly concerning, as it's unclear which configuration should take precedence. This could lead to unexpected behavior depending on how the YAML is parsed.

Would you like me to help identify the correct configuration and generate a cleanup patch to remove these duplicates?
</comment_end -->

Also applies to: 310-330

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/jade/resources/default_cfg/run_cfg.yml` around lines 189 - 209, There are
duplicate benchmark keys IPPE-DT and IPPE-CF; remove the repeated blocks and
consolidate into a single canonical entry for each (keep the IPPE-DT block once
and merge identical fields for codes, custom_input, nps, description,
only_input), and for IPPE-CF resolve the conflicting nps by choosing the correct
value (replace the two IPPE-CF blocks with one entry using the intended nps of
either 1e9 or 1e8), ensuring the merged block preserves codes, custom_input,
description and only_input from the authoritative source; finally run a YAML
linter to validate the file and confirm no duplicate keys remain.
🧹 Nitpick comments (1)
src/jade/resources/default_cfg/run_cfg.yml (1)

10-10: Verify resource impact of 10x NPS increase and consider adding validation.

Increasing NPS from 1e8 to 1e9 significantly increases computational time, memory consumption, and storage requirements. While the codebase shows awareness of extended execution times (see commented-out timeouts in benchmark.py), no bounds validation currently prevents excessive NPS values.

Consider:

  1. Confirming test and production environments can handle the extended execution
  2. Adding optional bounds validation in BenchmarkRunConfig to prevent accidentally setting extreme values

Note: The YAML file also contains duplicate benchmark entries (IPPE-DT at lines 189–198 and 310–319; IPPE-CF at lines 200–209 and 321–330) with inconsistent NPS values (1e9 vs 1e8 for IPPE-CF), which should be consolidated during cleanup.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/jade/resources/default_cfg/run_cfg.yml` at line 10, The NPS was bumped
from 1e8 to 1e9 causing potential resource/execution issues and
duplicate/inconsistent benchmark entries; add protection by implementing
optional bounds validation in BenchmarkRunConfig (e.g., add a max_nps parameter
and a validate_nps method called during construction or from_yaml parsing) that
raises or clamps values above a safe threshold, update benchmark.py to
respect/propagate this validation (and any timeout adjustments), and consolidate
duplicate IPPE-DT/IPPE-CF entries in run_cfg.yml so each benchmark has a single
canonical NPS value (ensure IPPE-CF isn’t left with inconsistent 1e8 vs 1e9
values).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/jade/resources/default_cfg/run_cfg.yml`:
- Around line 189-209: There are duplicate benchmark keys IPPE-DT and IPPE-CF;
remove the repeated blocks and consolidate into a single canonical entry for
each (keep the IPPE-DT block once and merge identical fields for codes,
custom_input, nps, description, only_input), and for IPPE-CF resolve the
conflicting nps by choosing the correct value (replace the two IPPE-CF blocks
with one entry using the intended nps of either 1e9 or 1e8), ensuring the merged
block preserves codes, custom_input, description and only_input from the
authoritative source; finally run a YAML linter to validate the file and confirm
no duplicate keys remain.

---

Nitpick comments:
In `@src/jade/resources/default_cfg/run_cfg.yml`:
- Line 10: The NPS was bumped from 1e8 to 1e9 causing potential
resource/execution issues and duplicate/inconsistent benchmark entries; add
protection by implementing optional bounds validation in BenchmarkRunConfig
(e.g., add a max_nps parameter and a validate_nps method called during
construction or from_yaml parsing) that raises or clamps values above a safe
threshold, update benchmark.py to respect/propagate this validation (and any
timeout adjustments), and consolidate duplicate IPPE-DT/IPPE-CF entries in
run_cfg.yml so each benchmark has a single canonical NPS value (ensure IPPE-CF
isn’t left with inconsistent 1e8 vs 1e9 values).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: af8f24c1-8ebd-4462-8851-0fda9aba900c

📥 Commits

Reviewing files that changed from the base of the PR and between b07b55e and 233a2e8.

📒 Files selected for processing (1)
  • src/jade/resources/default_cfg/run_cfg.yml

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 29, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

1 participant