fix(models): MLPRegression reads its hyperparameters from self.params, so HPO never reaches training - #873
Open
Maarmapa wants to merge 1 commit into
Open
Conversation
MLPRegression kept its configuration in two places: `self.params`, the dict of construction kwargs, which is what `train`, `save` and `load` read; and instance attributes, which is what ModelFactory._process_param and both optimizers write via `setattr(obj, key, value)`. A search over hidden_size, activation, learning_rate or epochs therefore never reached the network: every trial rebuilt the construction-time model, the study reported a best trial, and the model it produced had never been trained with the values that won. Nothing failed while that was true. Mirror the configuration onto instance attributes in `__init__` and read those everywhere. Two smaller defects fall out of the same split and are fixed here too: - `hidden_size` fell back to 100 in `train` and to 5 in `load`, against a schema that declares 16. `_CONFIG_DEFAULTS` is now the single home for every fallback, and a test pins it to the schema field by field. - `save` persisted the construction kwargs next to post-search weights, so reloading rebuilt a network of the wrong width. It now records the configuration the model actually ran with. Also fixes an AttributeError that made the model unusable outside ModelFactory: `train` reads `self.log_train_every_n_steps` and its three siblings, which `__init__` never set. Adds tests/back/models/test_mlp_regression.py, including one that drives a real OptunaOptimizer over hidden_size and asserts the widths of the networks actually built equal the widths Optuna suggested. Closes DashAISoftware#842 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BzZ6td4VJdRDe76UndWZY3
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.
MLPRegression kept its configuration in two places:
self.params, the dict of construction kwargs, which is whattrain,saveandloadread; and instance attributes, which is what ModelFactory._process_param and both optimizers write viasetattr(obj, key, value).A search over hidden_size, activation, learning_rate or epochs therefore never reached the network: every trial rebuilt the construction-time model, the study reported a best trial, and the model it produced had never been trained with the values that won. Nothing failed while that was true.
Mirror the configuration onto instance attributes in
__init__and read those everywhere. Two smaller defects fall out of the same split and are fixed here too:hidden_sizefell back to 100 intrainand to 5 inload, against a schema that declares 16._CONFIG_DEFAULTSis now the single home for every fallback, and a test pins it to the schema field by field.savepersisted the construction kwargs next to post-search weights, so reloading rebuilt a network of the wrong width. It now records the configuration the model actually ran with.Also fixes an AttributeError that made the model unusable outside ModelFactory:
trainreadsself.log_train_every_n_stepsand its three siblings, which__init__never set.Adds tests/back/models/test_mlp_regression.py, including one that drives a real OptunaOptimizer over hidden_size and asserts the widths of the networks actually built equal the widths Optuna suggested.
Closes #842
Claude-Session: https://claude.ai/code/session_01BzZ6td4VJdRDe76UndWZY3
Summary
Short explanation of what changed and why.
Type of Change
Check all that apply like this [x]:
Changes (by file)
Briefly list the important modified files and what was done.
Example:
path/to/file.py: descriptionanother/file.tsx: descriptionTesting (optional)
Only add if there's something reviewers should verify.
Notes (optional)
Additional context or considerations.