Conversation
Collaborator
Author
|
This PR cannot work: #109 has to be merged first. Then the current PR can be rebased on top of the master branch. |
Collaborator
Author
|
@sofianehaddad , @jschueller : I suggest to review this PR. |
jschueller
reviewed
May 7, 2026
jschueller
approved these changes
May 7, 2026
mbaudin47
commented
May 8, 2026
Collaborator
Author
|
mbaudin47
force-pushed
the
fixmorris
branch
3 times, most recently
from
395dd5c to
5428ebb
Compare
Collaborator
Author
|
Now that the implementation in otmorris is finalized (see openturns/otmorris#59), I have to update the current PR accordingly. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR refactors the Morris benchmark to use symbolic b0/b1/b2 parameters, adds a sparse vs non-sparse polynomial-chaos toggle propagated through the meta-algorithm, changes convergence/sample-size growth and timing behavior, updates examples and a reference script with timing/comparison, and adds a Morris throughput test. ChangesMorris Implementation, Polynomial Chaos Control, and Convergence Framework
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (2 warnings, 2 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
otbenchmark/_MorrisSensitivity.py (1)
402-581:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
b2deterministic coefficients length appears inconsistent with the enforced contract.Line 69 requires
len(b2Random) == 175, but the literal list in_get_parameters()appears to contain one extra value. If so, defaultMorrisSensitivity()will raiseValueErrorat construction time.Suggested hardening + fix pattern
- b2 = ot.Point( - [ + b2_values = [ ... - ] - ) - return b0, b1, b2 + ] + if len(b2_values) != 175: + raise ValueError(f"Expected 175 b2 coefficients, got {len(b2_values)}") + b2 = ot.Point(b2_values) + return b0, b1, b2🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@otbenchmark/_MorrisSensitivity.py` around lines 402 - 581, The literal Point b2 returned by _get_parameters() has an off-by-one length compared to the expected contract (len(b2Random) == 175) used by MorrisSensitivity(), so update _get_parameters() to ensure b2 has exactly 175 elements: locate the b2 definition in _get_parameters(), remove the extra numeric literal (or explicitly slice/construct the list to the first 175 values) and add/keep a runtime sanity check (e.g., assert or explicit len check) that raises a clear ValueError if the length is not 175 to prevent silent failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/scripts/compute_reference_Morris_indices.py`:
- Line 69: The print call uses an f-string with no placeholders (Ruff F541);
change the print in examples/scripts/compute_reference_Morris_indices.py (the
standalone print statement "Compare reference to computed indices") to a plain
string literal without the f prefix so it becomes print("Compare reference to
computed indices").
- Around line 86-93: The script computes comparison tables but leaves them as
bare expressions so nothing is printed when run as a script; update the code
that creates first_order_table and total_order_table (the calls to
comparisonSample with reference_first_order/result.first_order_indices and
reference_total_order/result.total_order_indices) to explicitly emit the tables
using print (e.g., print(first_order_table) and print(total_order_table)) after
they are created so the output appears in script mode.
In `@otbenchmark/_MorrisSensitivity.py`:
- Around line 318-321: The warning call in _MorrisSensitivity.py currently calls
warnings.warn(...) without stacklevel, causing incorrect caller attribution;
update the warnings.warn invocation (the one that emits "The parameters were
changed, but the reference Sobol' indices are not updated.") to include
stacklevel=2 so the warning points to the user call site instead of inside the
module.
- Around line 26-29: The example uses MorrisFunction(alpha, beta, gamma, b0)
which no longer matches the updated constructor and references undefined
variables; update the example in otbenchmark/_MorrisSensitivity.py to call the
new MorrisFunction constructor signature (or its zero-arg default) and/or define
the required parameters first so they exist, then wrap it in ot.Function as
before (i.e. replace MorrisFunction(alpha, beta, gamma, b0) with a call that
matches the current MorrisFunction API or MorrisFunction() and ensure
alpha/beta/gamma/b0 are either defined earlier or removed), and keep the
surrounding usage of ot.Function and input_sample unchanged.
In `@otbenchmark/_SparsePolynomialChaosSensitivityAnalysis.py`:
- Around line 139-147: The validation that checks candidate coefficients against
the training sample size is currently nested inside the verbose-only block; move
the guard (the if basis_dimension >= self.sample_size_train raise
ValueError(...) logic referencing basis_dimension and self.sample_size_train)
out of the verbose conditional so it always executes, while keeping the print
statements (that show self.sparse, self.total_degree, and basis_dimension)
inside the if verbose: block.
In `@tests/test_morris.py`:
- Around line 54-60: Replace the flaky hard perf assertion by using
time.perf_counter for more accurate timing, compute elapsed_time = t1 - t0
around model(input_sample), and compare score using a test assertion helper
(e.g., self.assertGreater) against a configurable threshold (read from an env
var or test config) instead of the hard `assert score > 1000`; alternatively
wrap the measurement as an opt-in benchmark (skip unless BENCHMARK=true) so CI
noise is avoided—update the code around t0, t1, elapsed_time, score and the
assertion accordingly.
---
Outside diff comments:
In `@otbenchmark/_MorrisSensitivity.py`:
- Around line 402-581: The literal Point b2 returned by _get_parameters() has an
off-by-one length compared to the expected contract (len(b2Random) == 175) used
by MorrisSensitivity(), so update _get_parameters() to ensure b2 has exactly 175
elements: locate the b2 definition in _get_parameters(), remove the extra
numeric literal (or explicitly slice/construct the list to the first 175 values)
and add/keep a runtime sanity check (e.g., assert or explicit len check) that
raises a clear ValueError if the length is not 175 to prevent silent failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 599f11f3-842b-4b99-bea1-245e7218bd58
📒 Files selected for processing (5)
.gitignoreexamples/scripts/compute_reference_Morris_indices.pyotbenchmark/_MorrisSensitivity.pyotbenchmark/_SparsePolynomialChaosSensitivityAnalysis.pytests/test_morris.py
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@otbenchmark/_MorrisSensitivity.py`:
- Around line 252-259: Update the provenance comment that documents how the
bundled Sobol' reference indices (firstOrderIndices and totalOrderIndices) were
generated so it matches the actual script
`examples/scripts/compute_reference_Morris_indices.py`: replace the 16384
train/validation description with the current high-fidelity settings (2**16
samples and sparse=False) and the corresponding details (polynomial chaos
expansion parameters, enumeration rule, degree, coefficient estimation method,
and achieved Q²) so the comment accurately reflects the workflow used to
regenerate those indices.
In `@otbenchmark/_SensitivityBenchmarkMetaAlgorithm.py`:
- Line 127: The docstring's parameter description for "sparse" is incorrect (it
says default False) while the function signature sets sparse=True; update the
docstring(s) to state that sparse defaults to True so they match the
implementation — locate the "sparse" parameter entry in the docstring(s) for the
relevant function or class in _SensitivityBenchmarkMetaAlgorithm.py (the same
docstring occurrences around the sparse=True signature and the separate
occurrence mentioned at lines ~148-149) and change the default note to "default:
True".
In `@otbenchmark/_SensitivityConvergence.py`:
- Around line 26-27: The constructor SensitivityConvergence.__init__ accepts a
sparse parameter but does not store it or forward it to computeError; update
__init__ to assign self.sparse = sparse and modify computeError (and any helper
methods it calls) to accept/use the sparse flag (propagate it through methods
like computeError and any internal calls) so the top-level sparse/non-sparse
configuration is honored by SensitivityConvergence.
- Around line 242-250: The try/except around self.computeError currently prints
errors and continues, which can let all repetitions fail and leave no data; wrap
the error logging behind self.verbose (only print if self.verbose) and increment
a local success counter inside the try when computeError returns successfully
(use the same scope where i, j, sample_size are available). After the
repetitions loop, check the success counter and if it is zero, raise a
RuntimeError (or otherwise stop the experiment) with a clear message including
sample_size and experiment index so you don't proceed with empty samples; apply
the identical change to the other try/except block that calls self.computeError
so both failure paths are guarded and terminal when no repetitions succeed.
- Line 254: The update to sample_size using int(sample_size *
self.sample_size_factor) can produce no growth when factor is marginally >1;
change the logic in the SensitivityConvergence code that updates sample_size so
it uses ceiling math and enforces monotonic growth: compute ceil(sample_size *
self.sample_size_factor) and then set sample_size to the greater of that value
and sample_size+1 (so sample_size always increases). Reference the sample_size
variable and self.sample_size_factor where the multiplication currently happens
and add the necessary math.ceil import/usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 649cf6ea-f248-4c9d-993e-b16e5a8033d8
📒 Files selected for processing (9)
.gitignoredoc/examples/sensitivity_methods/plot_convergence_ishigami.pyexamples/scripts/compute_reference_Morris_indices.pyotbenchmark/_MorrisSensitivity.pyotbenchmark/_SensitivityBenchmarkMetaAlgorithm.pyotbenchmark/_SensitivityConvergence.pyotbenchmark/_SparsePolynomialChaosSensitivityAnalysis.pytests/test_SensitivityConvergence.pytests/test_morris.py
🚧 Files skipped from review as they are similar to previous changes (4)
- .gitignore
- examples/scripts/compute_reference_Morris_indices.py
- tests/test_morris.py
- otbenchmark/_SparsePolynomialChaosSensitivityAnalysis.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
otbenchmark/_SensitivityConvergence.py (1)
245-254: 💤 Low valueConsider using explicit conversion flag in f-string.
Line 252 uses
str(e)within an f-string. Per Python style guidelines, use the explicit!sconversion flag instead.♻️ Suggested refinement
if verbose: print( - f"Error in experiment {i}, repetition {j}, sample size {sample_size}: {str(e)}" + f"Error in experiment {i}, repetition {j}, sample size {sample_size}: {e!s}" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@otbenchmark/_SensitivityConvergence.py` around lines 245 - 254, The f-string in the exception handler uses str(e); replace that explicit conversion with the Python f-string conversion flag by changing f"...{str(e)}" to f"...{e!s}" in the try/except block around the call to computeError (the block where first_order_AE, total_order_AE are assigned and variables i, j, sample_size and verbose are referenced) so the exception is converted using the recommended !s conversion flag.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@otbenchmark/_SensitivityConvergence.py`:
- Around line 245-254: The f-string in the exception handler uses str(e);
replace that explicit conversion with the Python f-string conversion flag by
changing f"...{str(e)}" to f"...{e!s}" in the try/except block around the call
to computeError (the block where first_order_AE, total_order_AE are assigned and
variables i, j, sample_size and verbose are referenced) so the exception is
converted using the recommended !s conversion flag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1af7c8d2-b848-443b-964e-1ea244a30eaa
📒 Files selected for processing (9)
.gitignoredoc/examples/sensitivity_methods/plot_convergence_ishigami.pyexamples/scripts/compute_reference_Morris_indices.pyotbenchmark/_MorrisSensitivity.pyotbenchmark/_SensitivityBenchmarkMetaAlgorithm.pyotbenchmark/_SensitivityConvergence.pyotbenchmark/_SparsePolynomialChaosSensitivityAnalysis.pytests/test_SensitivityConvergence.pytests/test_morris.py
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/test_SensitivityConvergence.py
- otbenchmark/_SensitivityBenchmarkMetaAlgorithm.py
- tests/test_morris.py
- doc/examples/sensitivity_methods/plot_convergence_ishigami.py
- examples/scripts/compute_reference_Morris_indices.py
- otbenchmark/_MorrisSensitivity.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@otbenchmark/_SensitivityConvergence.py`:
- Line 335: The code unpacks referenceLegend from _getConvergenceData but then
hardcodes the reference curve label r"$1/\sqrt{n}$" instead of using
referenceLegend (which should be r"$1/n$" for samplingMethod == "QMC"); update
the plotting call to use the referenceLegend variable for the curve legend
instead of the hardcoded string, ensuring the branch or value returned by
_getConvergenceData (and any logic in the function that depends on
samplingMethod) drives the displayed label in the function that generates the
plot in _SensitivityConvergence.py.
In `@otbenchmark/_SensitivityDistribution.py`:
- Around line 70-76: computeSampleIndices currently validates "Janon" in
__init__ but doesn't handle it in the estimator switch, causing a ValueError;
update computeSampleIndices to treat "Janon" the same as the existing branch for
"Jansen" by adding "Janon" to that conditional (reference: computeSampleIndices
and the estimator string "Janon"), so the sample index construction for "Jansen"
is reused for "Janon" and no unknown-estimator error is raised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 30d2e55a-f9fe-46a5-9672-fccd067c3e85
📒 Files selected for processing (5)
otbenchmark/_MorrisSensitivity.pyotbenchmark/_SensitivityConvergence.pyotbenchmark/_SensitivityDistribution.pytests/test_SensitivityConvergence.pytests/test_morris.py
🚧 Files skipped from review as they are similar to previous changes (1)
- otbenchmark/_MorrisSensitivity.py
mbaudin47
force-pushed
the
fixmorris
branch
2 times, most recently
from
6b060be to
d40874c
Compare
Closes #112 Updated Sobol' indices using PCE plot_convergence_ishigami.py: Fixed formatting Use CamelCase more consistently
Collaborator
Author
|
@jschueller , @efekhari27 : Can you please review this PR? |
jschueller
reviewed
Jun 17, 2026
|
|
||
| # %% | ||
| # Use polynomial chaos. | ||
| sparse = True # Otherwise, the PCE estimator is too slow for this example. |
Member
There was a problem hiding this comment.
did you mean False ? the comment is misleading
jschueller
reviewed
Jun 17, 2026
| def compute_sample_indices(self): | ||
| self.validSamplingMethods = {"MonteCarlo", "LHS", "QMC"} | ||
| if samplingMethod not in self.validSamplingMethods: | ||
| raise ValueError("Unknown value of sampling method : %s" % (samplingMethod)) |
jschueller
reviewed
Jun 17, 2026
| for i in range(dimension): | ||
| print( | ||
| "AE(S%d) = %.4f, AE(T%d) = %.4f" % (i, first_order_AE[i], i, total_order_AE[i]) | ||
| "AE(S%d) = %.4f, AE(T%d) = %.4f" % (i, firstOrderAE[i], i, totalOrderAE[i]) |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
otbenchmark/_SparsePolynomialChaosSensitivityAnalysis.py (1)
51-53:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the algorithm description for non-sparse mode.
The docstring still says this always uses LARS model selection, but
sparse=Falsenow switches toPenalizedLeastSquaresAlgorithmFactory(). Please make the description conditional onsparse.Suggested docstring update
- Uses LARS to select the model. + Uses LARS model selection when sparse=True. + Uses penalized least squares over the fixed basis when sparse=False. ... - Set to False to compute all coefficients. + Set to False to compute coefficients over the fixed candidate basis.Also applies to: 69-72
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@otbenchmark/_SparsePolynomialChaosSensitivityAnalysis.py` around lines 51 - 53, The docstring describing the algorithm selection is outdated and does not account for the conditional behavior based on the sparse parameter. Update the docstring to clarify that when sparse=True, the algorithm uses LARS for model selection, and when sparse=False, it uses PenalizedLeastSquaresAlgorithmFactory instead. Apply this conditional description update to all occurrences of the algorithm description in the docstring, including the sections at lines 51-53 and 69-72.otbenchmark/_SensitivityDistribution.py (1)
114-144:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
numberOfRepetitions > 0to avoid an unbound return path.If
numberOfRepetitionsis0, the loop never runs anddistributionFirst/distributionTotalare never assigned before return, which raises at runtime.Proposed fix
self.sampleSize = sampleSize self.numberOfRepetitions = numberOfRepetitions + if self.numberOfRepetitions <= 0: + raise ValueError( + f"numberOfRepetitions must be > 0, got {self.numberOfRepetitions}" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@otbenchmark/_SensitivityDistribution.py` around lines 114 - 144, The variables `distributionFirst` and `distributionTotal` are only assigned inside the for loop that iterates over `range(self.numberOfRepetitions)`. If `numberOfRepetitions` is 0, the loop never executes and these variables remain unbound, causing a runtime error when the return statement tries to access them. Add a validation check at the beginning of the method to ensure `self.numberOfRepetitions` is greater than 0, raising a ValueError if it is not, before entering the loop.
♻️ Duplicate comments (1)
otbenchmark/_SensitivityDistribution.py (1)
73-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign estimator support with the Janon-enabled API contract.
This class rejects
"Janon"in__init__and has no"Janon"execution path incomputeSampleIndices, while the benchmark meta-algorithm and tests accept"Janon". This creates an inconsistent public API across sensitivity entry points.Proposed fix
+from ._JanonSensitivityAlgorithm import JanonSensitivityAlgorithm + self.validEstimators = { "Saltelli", "Jansen", "Martinez", "MauntzKucherenko", + "Janon", } @@ elif self.estimator == "MauntzKucherenko": sobolAlgorithm = ot.MauntzKucherenkoSensitivityAlgorithm() + elif self.estimator == "Janon": + sobolAlgorithm = JanonSensitivityAlgorithm( + inputDesign, outputDesign, self.sampleSize + ) else: raise ValueError("Unknown value of estimator %s" % (self.estimator)) - sobolAlgorithm.setDesign(inputDesign, outputDesign, self.sampleSize) + if self.estimator != "Janon": + sobolAlgorithm.setDesign(inputDesign, outputDesign, self.sampleSize)Also applies to: 119-129
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@otbenchmark/_SensitivityDistribution.py` around lines 73 - 80, The _SensitivityDistribution class currently rejects the "Janon" estimator in the __init__ method's validEstimators set, but the benchmark API and tests expect it to be supported, creating an API inconsistency. Add "Janon" to the self.validEstimators set in __init__ to allow it as a valid estimator option. Additionally, implement the corresponding execution path for "Janon" in the computeSampleIndices method (around lines 119-129) to handle sample indices computation for this estimator, ensuring the class provides complete support for all advertised estimators.
🧹 Nitpick comments (1)
otbenchmark/_SparsePolynomialChaosSensitivityAnalysis.py (1)
105-111: ⚡ Quick winValidate the basis size before evaluating the training model.
The
basisDimension >= self.sampleSizeTraincheck now correctly runs outsideverbose, but it still happens after generatinginputTrainand evaluatingmodel(inputTrain). Invalid configurations should fail before potentially expensive model evaluations.Suggested restructuring
distribution = self.problem.getInputDistribution() model = self.problem.getFunction() dimension = distribution.getDimension() + distributionList = [distribution.getMarginal(i) for i in range(dimension)] + enumerateFunction = ot.HyperbolicAnisotropicEnumerateFunction( + dimension, self.hyperbolicQuasiNorm + ) + basisDimension = enumerateFunction.getBasisSizeFromTotalDegree(self.totalDegree) + if verbose: + print(f"> Sparse = {self.sparse}") + print(f"> Total degree = {self.totalDegree}") + print(f"> Basis dimension = {basisDimension}") + if basisDimension >= self.sampleSizeTrain: + raise ValueError( + f"The number of candidate coefficients is {basisDimension} " + f"is larger or equal to the sample size {self.sampleSizeTrain}" + ) if verbose: print(f"Generate train experiment, N={self.sampleSizeTrain}") sequence = ot.SobolSequence(dimension) experiment = ot.LowDiscrepancyExperiment( sequence, distribution, self.sampleSizeTrain @@ - distributionList = [distribution.getMarginal(i) for i in range(dimension)] if self.sparse: selectionAlgorithm = ot.LeastSquaresMetaModelSelectionFactory() else: selectionAlgorithm = ot.PenalizedLeastSquaresAlgorithmFactory() @@ - enumerateFunction = ot.HyperbolicAnisotropicEnumerateFunction( - dimension, self.hyperbolicQuasiNorm - ) multivariateBasis = ot.OrthogonalProductPolynomialFactory( polyColl, enumerateFunction ) - basisDimension = enumerateFunction.getBasisSizeFromTotalDegree(self.totalDegree) - if verbose: - print(f"> Sparse = {self.sparse}") - print(f"> Total degree = {self.totalDegree}") - print(f"> Basis dimension = {basisDimension}") - if basisDimension >= self.sampleSizeTrain: - raise ValueError( - f"The number of candidate coefficients is {basisDimension} " - f"is larger or equal to the sample size {self.sampleSizeTrain}" - ) adaptiveStrategy = ot.FixedStrategy(multivariateBasis, basisDimension)Also applies to: 135-145
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@otbenchmark/_SparsePolynomialChaosSensitivityAnalysis.py` around lines 105 - 111, The validation check for `basisDimension >= self.sampleSizeTrain` must be moved to occur before the creation of the LowDiscrepancyExperiment and the model evaluation with `model(inputTrain)`. Currently, expensive operations like generating the experiment sequence and evaluating the model happen before the basis dimension validation, which means invalid configurations waste computation. Move the validation check to the beginning of this section, before the sequence and experiment generation, so that invalid configurations fail fast without incurring unnecessary computational cost.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@doc/examples/sensitivity_methods/plot_benchmark_sensitivity_methods.py`:
- Around line 112-118: The Monte-Carlo and QMC estimation loops are not using
the current `estimator` variable correctly. In the Monte-Carlo loop, the
`metaSAAlgorithm.runSamplingEstimator()` call must pass the current `estimator`
as a parameter in addition to `sampleSize` to execute the intended estimator for
each iteration. In the QMC loop, the `name` variable should be assigned by
calling `estimator.getClassName()` instead of `sobolAlgorithm.getClassName()` to
ensure the printed name reflects the current estimator being evaluated, not a
previous scope variable.
- Around line 88-92: The print statements at lines 88-92 are using stale
variable names from a previous section instead of the newly computed QMC
results. Update the print statements to use the correct variable names that were
computed earlier in the QMC section. Replace the camelCase variable names
(computedFirstOrder, exactFirstOrder, computedTotalOrder, exactTotalOrder) with
the correct snake_case variable names (first_order and total_order) that match
what was computed in the preceding lines of the QMC calculation block.
In `@otbenchmark/_SensitivityConvergence.py`:
- Around line 335-336: The `referenceLegend` variable is unpacked from
`_getConvergenceData()` but not utilized in the `plotConvergenceGrid` method. At
line 367 (in the context of plotConvergenceGrid), replace the hardcoded string
`r"$1/\sqrt{n}$"` with the `referenceLegend` variable that was already
extracted. This ensures the correct reference curve label is used based on the
sampling method (e.g., `r"$1/n$"` for QMC instead of the hardcoded value),
making the behavior consistent with how `plotConvergenceCurve` correctly uses
`referenceLegend` at line 434.
- Around line 284-288: Add an early guard check to validate that sampleSizeTable
is not empty before calling getMin()[0] and getMax()[0] on it. Since
computeSobolSample can return an empty sample if all experiments fail, check the
size or length of sampleSizeTable at the beginning of this code block and raise
a meaningful error or handle the empty case appropriately if it has no data,
preventing the unclear exception that would result from calling getMin() and
getMax() on an empty ot.Sample object.
---
Outside diff comments:
In `@otbenchmark/_SensitivityDistribution.py`:
- Around line 114-144: The variables `distributionFirst` and `distributionTotal`
are only assigned inside the for loop that iterates over
`range(self.numberOfRepetitions)`. If `numberOfRepetitions` is 0, the loop never
executes and these variables remain unbound, causing a runtime error when the
return statement tries to access them. Add a validation check at the beginning
of the method to ensure `self.numberOfRepetitions` is greater than 0, raising a
ValueError if it is not, before entering the loop.
In `@otbenchmark/_SparsePolynomialChaosSensitivityAnalysis.py`:
- Around line 51-53: The docstring describing the algorithm selection is
outdated and does not account for the conditional behavior based on the sparse
parameter. Update the docstring to clarify that when sparse=True, the algorithm
uses LARS for model selection, and when sparse=False, it uses
PenalizedLeastSquaresAlgorithmFactory instead. Apply this conditional
description update to all occurrences of the algorithm description in the
docstring, including the sections at lines 51-53 and 69-72.
---
Duplicate comments:
In `@otbenchmark/_SensitivityDistribution.py`:
- Around line 73-80: The _SensitivityDistribution class currently rejects the
"Janon" estimator in the __init__ method's validEstimators set, but the
benchmark API and tests expect it to be supported, creating an API
inconsistency. Add "Janon" to the self.validEstimators set in __init__ to allow
it as a valid estimator option. Additionally, implement the corresponding
execution path for "Janon" in the computeSampleIndices method (around lines
119-129) to handle sample indices computation for this estimator, ensuring the
class provides complete support for all advertised estimators.
---
Nitpick comments:
In `@otbenchmark/_SparsePolynomialChaosSensitivityAnalysis.py`:
- Around line 105-111: The validation check for `basisDimension >=
self.sampleSizeTrain` must be moved to occur before the creation of the
LowDiscrepancyExperiment and the model evaluation with `model(inputTrain)`.
Currently, expensive operations like generating the experiment sequence and
evaluating the model happen before the basis dimension validation, which means
invalid configurations waste computation. Move the validation check to the
beginning of this section, before the sequence and experiment generation, so
that invalid configurations fail fast without incurring unnecessary
computational cost.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d658255c-9e84-4c04-88f6-35f1371d155c
📒 Files selected for processing (12)
.gitignoredoc/examples/sensitivity_methods/plot_benchmark_sensitivity_methods.pydoc/examples/sensitivity_methods/plot_convergence_ishigami.pyexamples/scripts/compute_reference_Morris_indices.pyotbenchmark/_MorrisSensitivity.pyotbenchmark/_SensitivityBenchmarkMetaAlgorithm.pyotbenchmark/_SensitivityConvergence.pyotbenchmark/_SensitivityDistribution.pyotbenchmark/_SparsePolynomialChaosSensitivityAnalysis.pytests/test_SensitivityBenchmarkMetaAlgorithm.pytests/test_SensitivityConvergence.pytests/test_morris.py
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/test_morris.py
- examples/scripts/compute_reference_Morris_indices.py
- otbenchmark/_MorrisSensitivity.py
- otbenchmark/_SensitivityBenchmarkMetaAlgorithm.py
- tests/test_SensitivityConvergence.py
- doc/examples/sensitivity_methods/plot_convergence_ishigami.py
Comment on lines
+88
to
+92
| print(f"Computed first order = {computedFirstOrder}") | ||
| print(f"Exact first order = {exactFirstOrder}") | ||
| # Total order | ||
| print("Computed total order = ", computed_total_order) | ||
| print("Exact total order = ", exact_total_order) | ||
| print(f"Computed total order = {computedTotalOrder}") | ||
| print(f"Exact total order = {exactTotalOrder}") |
There was a problem hiding this comment.
QMC report prints stale variables instead of the QMC result.
In this block, Line 81-Line 82 computes first_order/total_order, but Line 88-Line 92 prints computedFirstOrder/computedTotalOrder from a previous section.
Proposed fix
-print(f"Computed first order = {computedFirstOrder}")
+print(f"Computed first order = {first_order}")
print(f"Exact first order = {exactFirstOrder}")
@@
-print(f"Computed total order = {computedTotalOrder}")
+print(f"Computed total order = {total_order}")
print(f"Exact total order = {exactTotalOrder}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| print(f"Computed first order = {computedFirstOrder}") | |
| print(f"Exact first order = {exactFirstOrder}") | |
| # Total order | |
| print("Computed total order = ", computed_total_order) | |
| print("Exact total order = ", exact_total_order) | |
| print(f"Computed total order = {computedTotalOrder}") | |
| print(f"Exact total order = {exactTotalOrder}") | |
| print(f"Computed first order = {first_order}") | |
| print(f"Exact first order = {exactFirstOrder}") | |
| # Total order | |
| print(f"Computed total order = {total_order}") | |
| print(f"Exact total order = {exactTotalOrder}") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@doc/examples/sensitivity_methods/plot_benchmark_sensitivity_methods.py`
around lines 88 - 92, The print statements at lines 88-92 are using stale
variable names from a previous section instead of the newly computed QMC
results. Update the print statements to use the correct variable names that were
computed earlier in the QMC section. Replace the camelCase variable names
(computedFirstOrder, exactFirstOrder, computedTotalOrder, exactTotalOrder) with
the correct snake_case variable names (first_order and total_order) that match
what was computed in the preceding lines of the QMC calculation block.
Comment on lines
+112
to
+118
| computedFirstOrder, | ||
| computedTotalOrder, | ||
| ) = metaSAAlgorithm.runSamplingEstimator(sampleSize) | ||
| name = sobolAlgorithm.getClassName() | ||
| print(name) | ||
| print(" S = ", computed_first_order) | ||
| print(" T = ", computed_total_order) | ||
| print(" S = ", computedFirstOrder) | ||
| print(" T = ", computedTotalOrder) |
There was a problem hiding this comment.
Estimator loops currently don’t execute/report the intended estimator per iteration.
In the Monte-Carlo loop, Line 114 always calls the default estimator because estimator is not forwarded. In the QMC loop, printed name comes from sobolAlgorithm (previous scope) instead of the current estimator, so labels can be wrong.
Proposed fix
for sobolAlgorithm in estimators_list:
+ estimator = sobolAlgorithm.getClassName().replace("SensitivityAlgorithm", "")
(
computedFirstOrder,
computedTotalOrder,
- ) = metaSAAlgorithm.runSamplingEstimator(sampleSize)
- name = sobolAlgorithm.getClassName()
- print(name)
+ ) = metaSAAlgorithm.runSamplingEstimator(sampleSize, estimator=estimator)
+ print(estimator)
print(" S = ", computedFirstOrder)
print(" T = ", computedTotalOrder)
@@
- name = sobolAlgorithm.getClassName()
- print(name)
+ print(estimator)
print(f" S = {computedFirstOrder}")
print(f" T = {computedTotalOrder}")Also applies to: 124-132
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@doc/examples/sensitivity_methods/plot_benchmark_sensitivity_methods.py`
around lines 112 - 118, The Monte-Carlo and QMC estimation loops are not using
the current `estimator` variable correctly. In the Monte-Carlo loop, the
`metaSAAlgorithm.runSamplingEstimator()` call must pass the current `estimator`
as a parameter in addition to `sampleSize` to execute the intended estimator for
each iteration. In the QMC loop, the `name` variable should be assigned by
calling `estimator.getClassName()` instead of `sobolAlgorithm.getClassName()` to
ensure the printed name reflects the current estimator being evaluated, not a
previous scope variable.
Comment on lines
+284
to
+288
| sampleSizeInitial = sampleSizeTable.getMin()[0] | ||
| sampleSizeFinal = sampleSizeTable.getMax()[0] | ||
| sampleSizeLogArray = np.logspace( | ||
| np.log10(sampleSizeInitial), np.log10(sampleSizeFinal) | ||
| ) |
There was a problem hiding this comment.
Guard against empty sample before calling getMin()/getMax().
If all experiments fail in computeSobolSample, the returned sampleSizeTable will have size 0. Calling getMin()[0] or getMax()[0] on an empty ot.Sample will raise an exception with an unclear error message.
Consider adding an early guard:
Suggested fix
sampleSizeTable, firstOrderTable, totalOrderTable = self.computeSobolSample(
verbose=verbose
)
+ if sampleSizeTable.getSize() == 0:
+ raise RuntimeError(
+ "No convergence samples were collected. All experiments failed."
+ )
+
# Calcul des échelles logarithmiques pour la courbe de référence
sampleSizeInitial = sampleSizeTable.getMin()[0]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@otbenchmark/_SensitivityConvergence.py` around lines 284 - 288, Add an early
guard check to validate that sampleSizeTable is not empty before calling
getMin()[0] and getMax()[0] on it. Since computeSobolSample can return an empty
sample if all experiments fail, check the size or length of sampleSizeTable at
the beginning of this code block and raise a meaningful error or handle the
empty case appropriately if it has no data, preventing the unclear exception
that would result from calling getMin() and getMax() on an empty ot.Sample
object.
Comment on lines
+335
to
+336
| referenceLegend, | ||
| ) = self._getConvergenceData(verbose=verbose) |
There was a problem hiding this comment.
Use referenceLegend instead of hardcoded string.
referenceLegend is unpacked from _getConvergenceData but never used. Line 367 hardcodes r"$1/\sqrt{n}$" for the curve legend, which is incorrect when samplingMethod == "QMC" (should be r"$1/n$").
Note: plotConvergenceCurve correctly uses referenceLegend at line 434, but plotConvergenceGrid does not.
Proposed fix
curve = ot.Curve(sampleSizeArray, expectedConvergence)
- curve.setLegend(r"$1/\sqrt{n}$")
+ curve.setLegend(referenceLegend)
graph.add(curve)Also applies to: 367-367
🧰 Tools
🪛 Ruff (0.15.17)
[warning] 335-335: Unpacked variable referenceLegend is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@otbenchmark/_SensitivityConvergence.py` around lines 335 - 336, The
`referenceLegend` variable is unpacked from `_getConvergenceData()` but not
utilized in the `plotConvergenceGrid` method. At line 367 (in the context of
plotConvergenceGrid), replace the hardcoded string `r"$1/\sqrt{n}$"` with the
`referenceLegend` variable that was already extracted. This ensures the correct
reference curve label is used based on the sampling method (e.g., `r"$1/n$"` for
QMC instead of the hardcoded value), making the behavior consistent with how
`plotConvergenceCurve` correctly uses `referenceLegend` at line 434.
Source: Linters/SAST tools
jschueller
reviewed
Jun 17, 2026
|
|
||
| Caution: This changes the "SobolIndicesExperiment-SamplingMethod" | ||
| resource, which is used by the sampling estimators. | ||
|
|
Member
There was a problem hiding this comment.
its better not to write ResourceMap permanently: lets save and restore the value before/after the calls to SobolIndicesExperiment
jschueller
reviewed
Jun 17, 2026
|
|
||
| # Get the distribution | ||
| if is_first_simulation: | ||
| if isFirstSimulation: |
Member
There was a problem hiding this comment.
no need for isFirstSimulation var, thats just i == 0
jschueller
reviewed
Jun 17, 2026
jschueller
approved these changes
Jun 17, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
MorrisSensitivity.py:
Closes #112
Summary by CodeRabbit
New Features
Improvements
Tests
Chores