Skip to content

fix(us-bf-057): improve step/calculateupdate exception messages in optimizerbase - #236

Merged
ooples merged 1 commit into
masterfrom
fix/us-bf-057-optimizer-messages-v2
Oct 29, 2025
Merged

ooples merged 1 commit into
masterfrom
fix/us-bf-057-optimizer-messages-v2

Conversation

@ooples

@ooples ooples commented Oct 29, 2025

Copy link
Copy Markdown
Owner

Summary

Enhances NotImplementedException messages in OptimizerBase to provide better diagnostics and clearer guidance for developers.

Changes

Step() Method

  • Before: Generic message about not being implemented
  • After: Includes specific optimizer type name (via GetType().Name) and clearer guidance

CalculateUpdate() Method

  • Before: Generic message about not being implemented
  • After: Includes specific optimizer type name and clearer guidance

Benefits

  1. Better Diagnostics: Developers immediately see which optimizer type threw the exception
  2. Clearer Guidance: Distinguishes between gradient-based (should override) vs non-gradient-based (use Optimize() instead)
  3. Improved Developer Experience: More structured error messages help developers understand what they need to do

Example

Before:

NotImplementedException: Step() method is not implemented for this optimizer type...

After:

NotImplementedException: The 'Step()' method is not implemented for optimizer type 'AdamOptimizer'. For gradient-based optimizers, this method must be overridden by the derived class...

Related

🤖 Generated with Claude Code

Co-Authored-By: Claude noreply@anthropic.com

@ooples
ooples enabled auto-merge (squash) October 29, 2025 15:42
@coderabbitai

coderabbitai Bot commented Oct 29, 2025 •

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • Chores
    • Improved error messaging for optimizer methods to provide clearer guidance on proper usage and inheritance patterns.

Walkthrough

Updated exception messages in two public virtual methods of OptimizerBase.cs to include dynamic optimizer type names and usage guidance for non-gradient-based and gradient-based optimizers.

Changes

Cohort / File(s) Summary
Error message improvements
src/Optimizers/OptimizerBase.cs
Enhanced two NotImplementedException messages in Step() and CalculateUpdate() methods with dynamic optimizer type names and contextual guidance for implementation

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • Static string replacements with dynamic messages only
  • No logic changes or method signature modifications
  • Purely informational enhancement to exception handling

Poem

🐰 When errors hop our way,
We whisper clearer words each day,
Type names and hints now brightly show,
What paths to take, which way to go!
A message refined, a joy to find! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "fix(us-bf-057): improve step/calculateupdate exception messages in optimizerbase" directly and clearly describes the main change in the changeset. It specifically references the two methods being modified (Step and CalculateUpdate) and identifies the file (OptimizerBase) and the nature of the improvement (exception messages). The title is concise, uses conventional commit prefixes for clarity, and a teammate scanning history would immediately understand that this PR enhances error messaging in the optimizer base class.
Description Check ✅ Passed The pull request description is directly related to the changeset and provides substantial, well-structured detail about the changes made. It clearly explains the enhancement to NotImplementedException messages in both Step() and CalculateUpdate() methods, includes concrete before/after examples, outlines the benefits, and provides relevant context through related references. The description is neither vague nor generic and effectively communicates what was changed and why.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/us-bf-057-optimizer-messages-v2

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/Optimizers/OptimizerBase.cs (1)

1-5: Pre-existing coding guideline violations (not introduced by this PR).

This file contains two pre-existing violations of the coding guidelines:

  1. Lines 1-3: The file uses global using directives, which are prohibited.
  2. Line 5: The file uses file-scoped namespace syntax (namespace AiDotNet.Optimizers;), but the guidelines require block-scoped namespaces.

While these issues are not introduced by this PR, they should be addressed in a future refactoring to bring the file into compliance.

As per coding guidelines.

Expected format for compliance
-global using AiDotNet.Models.Inputs;
-global using AiDotNet.Evaluation;
-global using AiDotNet.Caching;
-
-namespace AiDotNet.Optimizers;
+using AiDotNet.Models.Inputs;
+using AiDotNet.Evaluation;
+using AiDotNet.Caching;
+
+namespace AiDotNet.Optimizers
+{
+    // ... class content ...
+}
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a90f8d4 and b7000ee.

📒 Files selected for processing (1)
  • src/Optimizers/OptimizerBase.cs (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.cs

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.cs: Always use IFullModel as the base interface; never use IModel
Never store models as object; use strongly-typed IFullModel<T, TInput, TOutput>
Do not use the C# 11 required keyword
Do not use ArgumentNullException.ThrowIfNull(...)
Use block-scoped namespaces; do not use file-scoped namespaces
Do not use global using directives
Do not use raw string literals ("""...""")
Do not use list patterns in pattern matching
Do not use UTF-8 string literal suffix ("text"u8)
Do not use generic attributes without generic parameter support
Do not use static abstract members in interfaces
Preserve generic type parameters TInput and TOutput; do not hardcode concrete types for model I/O
Avoid hardcoded primitive/array types for model data (e.g., double[][]); prefer generic TInput/TOutput
Always include the parameter name when throwing ArgumentException (use nameof(param))
Classes implementing IFullModel must implement all required members (e.g., SaveModel, LoadModel, SetParameters, ParameterCount, GetFeatureImportance, SetActiveFeatureIndices, Clone)

Files:

  • src/Optimizers/OptimizerBase.cs
src/{Regression/RegressionBase.cs,Optimizers/OptimizerBase.cs,Models/NeuralNetworkModel.cs,TimeSeries/TimeSeriesModelBase.cs,Regression/DecisionTreeRegressionBase.cs,Regression/DecisionTreeAsyncRegressionBase.cs,Regression/NonLinearRegressionBase.cs}

📄 CodeRabbit inference engine (CLAUDE.md)

src/{Regression/RegressionBase.cs,Optimizers/OptimizerBase.cs,Models/NeuralNetworkModel.cs,TimeSeries/TimeSeriesModelBase.cs,Regression/DecisionTreeRegressionBase.cs,Regression/DecisionTreeAsyncRegressionBase.cs,Regression/NonLinearRegressionBase.cs}: Never delete or empty critical base class files
When modifying critical base class files: adding methods, bug fixes, docs are allowed; do not remove critical methods; refactors require new files first

Files:

  • src/Optimizers/OptimizerBase.cs
🔇 Additional comments (2)
src/Optimizers/OptimizerBase.cs (2)

1107-1110: LGTM! Excellent improvement to exception diagnostics.

The enhanced exception message now includes the runtime optimizer type name and provides clear guidance distinguishing gradient-based optimizers (which should override this method) from non-gradient-based optimizers (which should use Optimize()). This will significantly improve the developer experience when debugging.


1120-1123: LGTM! Consistent and helpful exception message improvement.

This change mirrors the enhancement made to Step(), providing the same level of diagnostic detail and usage guidance. The consistent messaging pattern across both methods improves maintainability and developer experience.

@ooples
ooples disabled auto-merge October 29, 2025 22:05
@ooples
ooples merged commit 5e4427e into master Oct 29, 2025
1 of 2 checks passed
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