Repository navigation
fix(us-bf-022): implement IFullModel interface in VectorModel - #199
Merged
ooples merged 4 commits intoOct 23, 2025
Merged
Conversation
- Fix ModelMetadata<T> → ModelMetaData<T> casing throughout - Implement ParameterCount property returning Coefficients.Length - Implement SaveModel(string) method using Serialize() - Implement LoadModel(string) method using Deserialize() - Implement GetFeatureImportance() returning coefficient absolute values - Update _baseModel field from IModel to IFullModel - Update SetBaseModel method to use IFullModel interface - Update GetModelMetadata() → GetModelMetaData() method name All VectorModel interface implementation errors are now resolved. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR fully implements the IFullModel interface in VectorModel to resolve build errors and ensure compliance with project standards.
Key changes:
- Fixed casing from
ModelMetadata<T>toModelMetaData<T>throughout the file - Implemented missing
IFullModelinterface members:ParameterCount,SaveModel,LoadModel, andGetFeatureImportance - Updated
_baseModelandSetBaseModelto useIFullModelinstead ofIModel
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
- Add _cachedFeatureImportance field to cache GetFeatureImportance() results - Invalidate cache when coefficients change (Train, Deserialize, SetParameters, SetActiveFeatureIndices) - Wrap SaveModel in try-catch with specific error messages for access denied, directory not found, and IO errors - Wrap LoadModel in try-catch with specific error messages for access denied, IO errors, and invalid format Resolves PR #199 comments on lines 459, 623, and 660. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…lmodel-vectormodel
Contributor
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
…null safety - Return defensive copy of cached dictionary in GetFeatureImportance() to prevent external mutation (line 1061) - Also return copy when creating new cache to maintain consistency (line 1070) - Initialize _baseModel field to null with nullable type to prevent NullReferenceException (line 1078) These changes fix 3 critical issues: 1. Security: Prevents callers from mutating cached feature importance 2. Performance: Maintains existing caching optimization 3. Null safety: Properly initializes _baseModel field 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
ooples
added a commit
that referenced
this pull request
Oct 23, 2025
* fix(US-BF-020): Implement persistence + feature-aware delegations in MappedRandomForestModel * fix(US-BF-016): Implement SetParameters for ExpressionTree (assigns values to constant nodes) * fix(US-BF-020): serialize wrapper metadata + base model; robust deserialize; map feature importance keys via mapper reflection when available * fix(US-BF-020): implement wrapper-aware SaveModel/LoadModel (container format) for mapped RF model * fix(US-BF-020): cache inverse-map reflection and log mapping exceptions * refactor(US-BF-020): extract wrapper (de)serialization helpers to remove duplication and validate target features * refactor(US-BF-020): init inverse-map MethodInfo in ctor; handle null Invoke result safely * fix: address code review feedback - remove console logging, extract magic constant, fix brace formatting, improve LoadModel error handling * fix: add null checks and parameter count validation to SetParameters * refactor(us-bf-016): address copilot review comments in SetParameters - Add validation after parameter assignment to ensure all parameters were consumed - Add comment explaining why two traversals are necessary for atomicity - Refactor Assign to return new index instead of mutating closure variable for better thread-safety - Rename Assign to AssignAndReturnNextIndex to reflect new behavior 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: improve exception handling and logging in transferrandomforest - Add Console.WriteLine for inverse feature mapping failures with context - Add specific exception handlers for mapping confidence (InvalidCastException, FormatException) - Add Debug.WriteLine for wrapper deserialization failures with expected exception types - Document why exceptions are handled and what fallback behavior is used - Resolves PR #155 code review comments on exception documentation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: remove worktrees directories from version control 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: cache parametercount to avoid expensive tree traversal on each access Added _parameterCount nullable field to cache the parameter count value, similar to the existing _featureCount caching pattern. This prevents expensive tree traversal on every ParameterCount property access. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: address all unresolved PR comments for #155 - ExpressionTree.cs: Replace new Random() with thread-safe ThreadRandom property (fixes 5 locations at lines 313, 372, 415, 450, 483) - TransferRandomForest.cs: Add detailed exception documentation and logging for all catch blocks - Inverse mapping: Added expected exception types and debug logging - Mapping confidence: Split bare catch into specific exception types (InvalidCastException, FormatException) with logging - Wrapper deserialization: Added exception documentation and debug logging 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * chore: remove backup files * fix(us-bf-016): address code review feedback for ExpressionTree - Use shared static Random instance instead of creating new instances (improves randomness quality) - Add nullable annotations to CountConstants and AssignAndReturnNextIndex parameters - Add comprehensive XML documentation for SetParameters method clarifying in-place mutation - Add documentation for ParameterCount property explaining on-demand calculation - Resolve all remaining unresolved review comments 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: address all copilot comments in ExpressionTree - thread-safe random, nullable annotations, caching * fix: add nullable annotations to SetParameters helper methods - Add nullable annotation to CountConstants parameter - Add nullable annotation to AssignAndReturnNextIndex parameter - Improves type safety and addresses Copilot review comments 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: address all remaining copilot comments in ExpressionTree (8 fixes) - Remove null-forgiving operators (!) from _random.Value throughout file - Add DisposeRandomGenerator() method to cleanup ThreadLocal<Random> - Update ThreadLocal<Random> documentation to clarify thread safety - Make CountConstants local function not check for null (only called with non-null 'this') - Make AssignAndReturnNextIndex local function not check for null (only called with non-null nodes) - Update SetParameters documentation to emphasize in-place mutation vs other methods - Update ParameterCount documentation to say it uses Coefficients property - Add comments to local functions explaining non-null assumptions 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix(us-bf-022): implement IFullModel interface in VectorModel - Fix ModelMetadata<T> → ModelMetaData<T> casing throughout - Implement ParameterCount property returning Coefficients.Length - Implement SaveModel(string) method using Serialize() - Implement LoadModel(string) method using Deserialize() - Implement GetFeatureImportance() returning coefficient absolute values - Update _baseModel field from IModel to IFullModel - Update SetBaseModel method to use IFullModel interface - Update GetModelMetadata() → GetModelMetaData() method name All VectorModel interface implementation errors are now resolved. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: add caching and improve error messages in VectorModel - Add _cachedFeatureImportance field to cache GetFeatureImportance() results - Invalidate cache when coefficients change (Train, Deserialize, SetParameters, SetActiveFeatureIndices) - Wrap SaveModel in try-catch with specific error messages for access denied, directory not found, and IO errors - Wrap LoadModel in try-catch with specific error messages for access denied, IO errors, and invalid format Resolves PR #199 comments on lines 459, 623, and 660. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: add feature importance caching, prevent cache mutation, and add null safety - Return defensive copy of cached dictionary in GetFeatureImportance() to prevent external mutation (line 1061) - Also return copy when creating new cache to maintain consistency (line 1070) - Initialize _baseModel field to null with nullable type to prevent NullReferenceException (line 1078) These changes fix 3 critical issues: 1. Security: Prevents callers from mutating cached feature importance 2. Performance: Maintains existing caching optimization 3. Null safety: Properly initializes _baseModel field 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: restore null checks in SetParameters to prevent NullReferenceException This commit addresses 6 critical issues identified in PR #155 review comments: 1. DisposeRandomGenerator() - Already exists at line 143-146, prevents resource leaks 2. ParameterCount documentation - Clarified that value comes from Coefficients property 3. CountConstants - RESTORED null check at method entry (line 1199) 4. AssignAndReturnNextIndex - RESTORED null check at method entry (line 1221) 5. CountConstants recursive calls - Already had null checks before recursion 6. AssignAndReturnNextIndex recursive calls - Already had null checks before recursion The critical fixes prevent NullReferenceException by ensuring both local functions handle null nodes properly before attempting to access node properties. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: remove redundant null initialization in VectorModel _baseModel field 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Franklin Moormann <cheatcountry@gmail.com> * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Franklin Moormann <cheatcountry@gmail.com> * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Franklin Moormann <cheatcountry@gmail.com> * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Franklin Moormann <cheatcountry@gmail.com> * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Franklin Moormann <cheatcountry@gmail.com> * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Franklin Moormann <cheatcountry@gmail.com> * Update src/LinearAlgebra/ExpressionTree.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Franklin Moormann <cheatcountry@gmail.com> * Update src/LinearAlgebra/ExpressionTree.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Franklin Moormann <cheatcountry@gmail.com> * Update src/LinearAlgebra/ExpressionTree.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Franklin Moormann <cheatcountry@gmail.com> * Update src/LinearAlgebra/ExpressionTree.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Franklin Moormann <cheatcountry@gmail.com> --------- Signed-off-by: Franklin Moormann <cheatcountry@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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
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.
Summary
Fully implements IFullModel interface in VectorModel to resolve build errors and adhere to project standards.
Changes:
ModelMetadata<T>→ModelMetaData<T>casing throughout VectorModel.csParameterCountproperty returningCoefficients.LengthSaveModel(string filePath)method for serializationLoadModel(string filePath)method for deserializationGetFeatureImportance()method returning coefficient absolute values_baseModelandSetBaseModelto useIFullModelinstead ofIModel(project guideline compliance)Build Status:
User Story: US-BF-022
🤖 Generated with Claude Code