Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an optimoptions builtin: shared canonicalization, descriptor and GPU/fusion metadata, async entrypoint that builds and validates solver-specific option structs for fminbnd/fzero/fsolve, tests, JSON docs, and refactors optimset to use the shared helper. Changesoptimoptions builtin implementation
sequenceDiagram
participant User
participant OptimoptionsCall as optimoptions
participant ParseSolver
participant InitDefaults
participant ApplyPairs
participant ValidateFields
participant SolverFn as fminbnd/fzero/fsolve
User->>OptimoptionsCall: optimoptions('fminbnd', 'TolX', 1e-6)
OptimoptionsCall->>ParseSolver: determine active solver
ParseSolver->>InitDefaults: initialize solver defaults
InitDefaults->>ApplyPairs: merge and apply pairs
ApplyPairs->>ValidateFields: canonicalize and validate
ValidateFields->>OptimoptionsCall: set struct field
OptimoptionsCall->>User: return options struct
User->>SolverFn: fminbnd(fun, a, b, opts)
SolverFn->>SolverFn: use TolX, MaxIter from opts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ef2b2e6. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@crates/runmat-runtime/src/builtins/math/optim/optimoptions.rs`:
- Around line 233-239: The code resets options to defaults whenever a later
struct specifies a non-generic solver even if that solver is the same as the
currently active one, wiping prior overrides; change the logic in the
Value::Struct handling so you call solver_from_options(&existing) into
next_solver and only replace options = default_options(next_solver) when
next_solver != solver (the currently active solver), then set solver =
next_solver and call apply_struct_fields(&existing, &mut options, solver) so
same-solver merges preserve prior overrides while switches to a different solver
reset to defaults.
- Around line 470-478: The function positive_integer_scalar currently casts a
validated f64 to usize which on stable Rust saturates and can produce usize::MAX
for out-of-range inputs; modify positive_integer_scalar to explicitly reject
parsed values that are >= 2^(usize::BITS) (or >= (2f64.powi(usize::BITS as
i32))) before casting so that it returns an
OPTIMOPTIONS_ERROR_INVALID_OPTION_VALUE via optimoptions_error_with when parsed
is too large, keeping the existing integer check (parsed.fract()==0.0) and only
performing Ok(parsed as usize) after the new upper-bound check.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 481ace3e-f969-42a2-bdde-5cc2405e010c
📒 Files selected for processing (5)
crates/runmat-runtime/src/builtins/builtins-json/optimoptions.jsoncrates/runmat-runtime/src/builtins/math/optim/common.rscrates/runmat-runtime/src/builtins/math/optim/mod.rscrates/runmat-runtime/src/builtins/math/optim/optimoptions.rscrates/runmat-runtime/src/builtins/math/optim/optimset.rs
Contributor
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 `@crates/runmat-runtime/src/builtins/math/optim/optimoptions.rs`:
- Around line 255-265: When transitioning from Solver::Generic to a concrete
solver (when next_solver != Solver::Generic and solver == Solver::Generic), do
not discard the already-accumulated generic fields in options; instead create
the concrete solver defaults via default_options(next_solver) and then
merge/overlay any fields present in the current generic options that are valid
for next_solver into that new options object before setting skip_defaults_from
and calling apply_struct_fields; implement this by copying applicable fields
from the existing options (the variable options that was accumulating generic
overrides) into the new default_options(next_solver) instance (or by a small
helper like merge_generic_into_defaults), then proceed to set solver =
next_solver, skip_defaults_from = None, and call apply_struct_fields(&existing,
&mut options, solver, true, skip_defaults_from)? to reapply struct fields
correctly.
- Around line 452-458: The comparison currently checks raw Value equality
(default == value) and should instead compare normalized/coerced values; before
calling lookup_case_insensitive(...).is_some_and(|default| default == value) run
the incoming key/value through the same validation/coercion used by
set_option_field (i.e., normalize the field using the set_option_field path or
its helper), then compare the normalized incoming Value to the stored default;
update the block around canonical_option_name, lookup_case_insensitive,
source_defaults, key and value to use the normalized value for the equality
check so semantically equivalent inputs (e.g., Int vs Num, CharArray vs String)
are treated as defaults and skipped.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f00ca36a-179d-42cb-b4cd-53e22ecdaf47
📒 Files selected for processing (1)
crates/runmat-runtime/src/builtins/math/optim/optimoptions.rs
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
1 participant
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.
Note
Low Risk
New additive API with host-only metadata; existing solvers consume the same option fields already read via common helpers, and behavior is heavily covered by tests.
Overview
Adds a new
optimoptionsbuiltin that builds and updates solver-specific option structs forfminbnd,fzero, andfsolve, with MATLAB-style call forms (solver only, solver + name/value pairs, or existing struct + updates/merges).Validation is stricter than
optimset: solver-specific defaults, case-insensitive canonical field names, rejected unknown options, and typed checks for tolerances, integer limits, andDisplay. Struct merges handle solver switches, generic options, and skipping fields that match normalized defaults.gpuArrayscalars are gathered on the host; GPU/fusion specs mark the builtin as host metadata.canonical_option_nameis moved intocommon.rsand shared withoptimset. Builtin JSON docs and broad unit/integration tests (including passing options into the three solvers) are included.Reviewed by Cursor Bugbot for commit 32e7741. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit