Skip to content

fix(us-ci-004): refine TransferLearning cross-domain exception messages - #225

Merged
ooples merged 1 commit into
merge-dev2-to-masterfrom
fix/us-ci-004-transfer-learning-cross-domain
Oct 25, 2025
Merged

ooples merged 1 commit into
merge-dev2-to-masterfrom
fix/us-ci-004-transfer-learning-cross-domain

Conversation

@ooples

@ooples ooples commented Oct 24, 2025

Copy link
Copy Markdown
Owner

Summary

  • Refined InvalidOperationException messages in TransferNeuralNetwork.cs and TransferRandomForest.cs
  • Simplified messages to align with user story Option 2 guidance
  • Clearer API direction: directs users to public Transfer() method that accepts source data
  • Removed redundant technical details about API limitations
  • Messages now more concise and actionable

Changes Made

TransferNeuralNetwork.cs:

  • Updated TransferCrossDomain exception message to be more concise
  • Focuses on the solution (use public Transfer method) rather than the problem

TransferRandomForest.cs:

  • Updated TransferCrossDomain exception message to match TransferNeuralNetwork
  • Consistent messaging across both transfer learning implementations

Test Plan

  • Build passes with 0 errors in modified files (net8.0 target)
  • Exception messages are clear and actionable
  • Public Transfer() methods provide proper cross-domain functionality
  • Transfer learning API works correctly across domains (existing functionality)

Acceptance Criteria Met

  • TransferCrossDomain methods throw InvalidOperationException (not NotImplementedException)
  • Exception messages provide clear explanation and guidance
  • API for cross-domain transfer learning is clear and usable via public Transfer() method

🤖 Generated with Claude Code

- Simplified InvalidOperationException messages in TransferNeuralNetwork and TransferRandomForest
- Aligned with user story Option 2 guidance for clearer API direction
- Removed technical details about API limitations
- Direct users to public Transfer() method that accepts source data
- Build verified: 0 errors, 0 warnings

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 24, 2025 •

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • Bug Fixes
    • Improved error messages in transfer learning algorithms to provide clearer guidance when cross-domain transfers are invoked, directing users to use the appropriate public methods.

Walkthrough

Exception messages in two transfer learning algorithm classes are updated to provide clearer guidance. Users attempting cross-domain transfer through protected methods are now directed to use the public Transfer method that accepts both source and target data.

Changes

Cohort / File(s) Summary
Cross-domain transfer exception messages
src/TransferLearning/Algorithms/TransferNeuralNetwork.cs, src/TransferLearning/Algorithms/TransferRandomForest.cs
Updated InvalidOperationException messages in TransferCrossDomain methods to clarify that cross-domain transfer requires the public Transfer method accepting both source and target data, rather than the protected path

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • Exception message text updates only; no logic changes
  • Identical pattern applied consistently across two similar classes

Poem

🐰 A clearer path the rabbit spied,
Exception whispers, gently guide,
"Use public Transfer!" the message sings,
No mysteries—just helpful things! 🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title "fix(us-ci-004): refine TransferLearning cross-domain exception messages" directly and accurately describes the main change in the pull request. The raw summary confirms that both modified files (TransferNeuralNetwork.cs and TransferRandomForest.cs) had their InvalidOperationException messages refined for clarity and to better guide users to the public Transfer() method. The title is specific, concise, and clearly communicates the primary change without vague language or unnecessary details.
Description Check ✅ Passed The pull request description is well-structured and directly related to the changeset. It provides a clear summary of what was refined (exception messages in TransferNeuralNetwork.cs and TransferRandomForest.cs), explains the rationale (aligning with user story guidance, removing redundant details, making messages more actionable), and outlines specific changes in each file along with test plans and acceptance criteria. The description demonstrates meaningful engagement with the changes and their purpose.
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-ci-004-transfer-learning-cross-domain

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0fdae03 and 9bcf6b7.

📒 Files selected for processing (2)
  • src/TransferLearning/Algorithms/TransferNeuralNetwork.cs (1 hunks)
  • src/TransferLearning/Algorithms/TransferRandomForest.cs (1 hunks)
🔇 Additional comments (2)
src/TransferLearning/Algorithms/TransferNeuralNetwork.cs (1)

60-61: LGTM! Exception message is clear and actionable.

The refined exception message effectively explains the limitation and directs users to the correct public method with the proper signature. This improves the developer experience by making the cross-domain transfer API clearer.

src/TransferLearning/Algorithms/TransferRandomForest.cs (1)

81-82: LGTM! Consistent messaging across transfer learning implementations.

The exception message matches the one in TransferNeuralNetwork.cs, ensuring consistent guidance across both transfer learning algorithms. This consistency improves the API's usability and helps users understand the correct pattern for cross-domain transfer.


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

@ooples

ooples commented Oct 24, 2025

Copy link
Copy Markdown
Owner Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Oct 24, 2025

Copy link
Copy Markdown
Contributor
✅ Actions performed

Full review triggered.

@ooples
ooples merged commit 855873c into merge-dev2-to-master Oct 25, 2025
1 of 3 checks passed
@ooples
ooples deleted the fix/us-ci-004-transfer-learning-cross-domain branch October 25, 2025 14:18
ooples added a commit that referenced this pull request Feb 6, 2026
…e in initializerandomsolution

The optimizer's _model field was null when InitializeRandomSolution accessed
model.ParameterCount because the model was never set on the optimizer after
configuration.

Changes:
- Add SetModel(IFullModel) method to IOptimizer interface
- Implement SetModel in OptimizerBase to set the _model field
- Implement SetModel in ShardedOptimizerBase to delegate to wrapped optimizer
- Call SetModel in AiModelBuilder before Optimize() in all training paths:
  - Regular training path (line 2107)
  - Hyperparameter optimization trial (line 1940)
  - Knowledge distillation fallback (line 5269)
  - Deep ensemble member optimizer (line 6793)

This fixes Issue #225: [Fix] Kill Time Series Stubs & Implement Real Training
by ensuring the model is always available to the optimizer before training.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
ooples added a commit that referenced this pull request Feb 6, 2026
…in InitializeRandomSolution (#823)

* fix: add setmodel method to ioptimizer interface to fix null reference in initializerandomsolution

The optimizer's _model field was null when InitializeRandomSolution accessed
model.ParameterCount because the model was never set on the optimizer after
configuration.

Changes:
- Add SetModel(IFullModel) method to IOptimizer interface
- Implement SetModel in OptimizerBase to set the _model field
- Implement SetModel in ShardedOptimizerBase to delegate to wrapped optimizer
- Call SetModel in AiModelBuilder before Optimize() in all training paths:
  - Regular training path (line 2107)
  - Hyperparameter optimization trial (line 1940)
  - Knowledge distillation fallback (line 5269)
  - Deep ensemble member optimizer (line 6793)

This fixes Issue #225: [Fix] Kill Time Series Stubs & Implement Real Training
by ensuring the model is always available to the optimizer before training.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* fix: correct arima predict vector bounds for edge cases

- Use _arCoefficients.Length instead of LagOrder for lastObservedValues
  to ensure dot product vectors have matching lengths
- Guard lastObservedValues[0] access when P=0 (pure MA model)
- Guard lastErrors[0] access when Q=0 (pure AR model)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* fix: address PR review comments and add SetModel to all test optimizers

- Add SetModel implementation to all test optimizer classes:
  - FederatedDeterministicDeltaOptimizer
  - FederatedNoOpOptimizer
  - SinglePassTestOptimizer
  - ResetTrackingOptimizer
  - MockOptimizer
  - PassthroughOptimizer
  - SingleStepTrainOptimizer
  - DeterministicNeuralNetworkParameterOptimizer

- Address PR review comments:
  - Improve SetModel comments in AiModelBuilder to clarify purpose
  - Add Reset() before SetModel in KD fallback path for consistency
  - Make SetModel virtual in OptimizerBase with OnModelChanged hook
  - Add breaking change documentation to IOptimizer.SetModel

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* fix: address remaining PR review comments

- Reorder HPO objective function: apply trial hyperparameters BEFORE Reset()
  so adaptive state is initialized from new hyperparameters
- Improve SetModel comment wording to not imply Reset() clears the model
- Update OnModelChanged signature to pass oldModel and newModel parameters
  allowing derived classes to react to model changes

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* chore: add coderabbit configuration with facade pattern guidelines

Configure CodeRabbit code reviews to enforce:
- Facade pattern: users should only use AiModelBuilder and AiModelResult
- Access modifier guidelines: prefer internal over public to hide IP
- Review checklist for maintaining clean public API surface

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* fix: correct MaternKernel parameter name in playground example

The matern-kernel playground example used 'length' but the constructor
parameter is named 'lengthScale', causing compilation test failure.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: add RequireModel() guard to replace null-forgiving _model! access

Adds a protected RequireModel() helper that returns the model or throws
InvalidOperationException with a clear message pointing to SetModel().
Replaces all _model! usages with RequireModel() for better diagnostics.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
Co-authored-by: franklinic <franklin@ivorycloud.com>
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