Skip to content

Add option to pick toy#1200

Merged
anigamova merged 2 commits intomainfrom
nckw_add_pickToy_option
Jan 29, 2026
Merged

Add option to pick toy#1200
anigamova merged 2 commits intomainfrom
nckw_add_pickToy_option

Conversation

@nucleosynthesis
Copy link
Copy Markdown
Contributor

@nucleosynthesis nucleosynthesis commented Jan 28, 2026

When running with --toys N, can pick a specific toy to run over with --pickToy i, where i = 1..N. Allows used to pick out a toy previously generated with -M GenerateOnly.

Also means global observables are correctly picked up too.

Summary by CodeRabbit

  • New Features

    • Added a command-line option to select a specific toy for processing during toy MC runs.
    • The selected toy is applied when toy runs are enabled; default behavior is unchanged when not set.
  • Bug Fixes

    • Validation added to ensure the chosen toy index is within the valid range; errors abort invalid requests.
  • Documentation

    • Updated help/docs to reflect the new option.

✏️ Tip: You can customize this high-level summary in your review settings.

When running with --toys N, can pick a specific toy to run over with
--pickToy i, where i = 1..N. Allows used to pick out a toy previously
generated with -M GenerateOnly. Also means global observables are
correctly picked up too.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Jan 28, 2026

📝 Walkthrough

Walkthrough

Adds a toy-picking feature: a new --pickToy CLI option validates the chosen toy index against runToys and, if valid, propagates it to the combiner via Combine::setPickToy(). Toy-generation loops are changed to process only the selected toy when set.

Changes

Cohort / File(s) Summary
CLI Option Binding
bin/combine.cpp
Adds --pickToy integer option, consolidates toys/seed/hintMethod options into a chained block, validates pickToy is within 1..runToys when runToys>0, errors/aborts on invalid input, and calls combiner.setPickToy(pickToy).
Public Interface
interface/Combine.h
Declares new global extern int pickToy_ and adds public static method Combine::setPickToy(int pickToy) to expose toy selection.
Implementation
src/Combine.cc
Defines pickToy_ (default 0) and implements Combine::setPickToy(). Alters toy-generation loops to skip all toys except the one specified when pickToy_ != 0.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I nibble code lines, one by one,
I pick a toy beneath the sun,
A chosen hop, the others cease,
Validation snug — now tests find peace,
🥕✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Add option to pick toy' accurately reflects the main change: introducing a new --pickToy command-line option to select a specific toy when running with --toys N.

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

✨ Finishing touches
  • 📝 Generate docstrings

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

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.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@src/Combine.cc`:
- Around line 1031-1032: The formatting around the conditional using pickToy_
and iToy is failing the clang-format check; run git-clang-format HEAD~ to
auto-fix formatting or manually reformat the statement in Combine.cc so it
matches project clang-format rules (e.g., spacing around parentheses and
operators) for the expression "if ((pickToy_ != 0) && (iToy != pickToy_))
continue;" ensuring no logic changes to pickToy_ and iToy handling.
🧹 Nitpick comments (1)
bin/combine.cpp (1)

209-215: Consider warning when --pickToy is used without --toys.

The validation correctly rejects out-of-range values when runToys > 0. However, if a user specifies --pickToy 5 without --toys (i.e., runToys == 0), no error or warning is issued, and the option is silently ignored. This could lead to user confusion.

Consider adding a warning:

💡 Suggested enhancement
  if (runToys > 0  ){
    if ( ( !vm["pickToy"].defaulted() ) && ( pickToy > runToys || pickToy <= 0) ){
      std::cerr << "ERROR - set pickToy to values 1 to (N toys) cannot use pickToy=" << pickToy << std::endl;
      assert(0);
    }
+ } else if ( !vm["pickToy"].defaulted() && pickToy != 0 ) {
+   std::cerr << "WARNING - --pickToy is ignored when not using --toys" << std::endl;
  }
  combiner.setPickToy(pickToy);

@codecov
Copy link
Copy Markdown

codecov bot commented Jan 28, 2026

Codecov Report

❌ Patch coverage is 78.57143% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 20.71%. Comparing base (1b0929d) to head (511ecc9).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
bin/combine.cpp 81.81% 2 Missing ⚠️
src/Combine.cc 66.66% 1 Missing ⚠️

❌ Your patch status has failed because the patch coverage (78.57%) is below the target coverage (98.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1200      +/-   ##
==========================================
+ Coverage   20.69%   20.71%   +0.01%     
==========================================
  Files         195      195              
  Lines       26156    26166      +10     
  Branches     3923     3924       +1     
==========================================
+ Hits         5413     5420       +7     
- Misses      20743    20746       +3     
Files with missing lines Coverage Δ
interface/Combine.h 100.00% <ø> (ø)
src/Combine.cc 50.17% <66.66%> (+0.05%) ⬆️
bin/combine.cpp 66.95% <81.81%> (+0.14%) ⬆️
Files with missing lines Coverage Δ
interface/Combine.h 100.00% <ø> (ø)
src/Combine.cc 50.17% <66.66%> (+0.05%) ⬆️
bin/combine.cpp 66.95% <81.81%> (+0.14%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@anigamova
Copy link
Copy Markdown
Collaborator

Looks good, not sure why lint is suddenly complaining about everything not related to this PR.
Would be also really good to implement the CI test with toys, which as I realized thanks to this PR we don't have at all, but it does not have to be here

ran git-clang-format HEAD~ as suggested by coderabbitai
@anigamova anigamova merged commit 7d8ee9d into main Jan 29, 2026
15 of 17 checks passed
@anigamova anigamova deleted the nckw_add_pickToy_option branch January 29, 2026 07:59
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.

2 participants