[feature] #1191 is trained property to indicate the state of the model.#1232
[feature] #1191 is trained property to indicate the state of the model.#1232timruhkopf wants to merge 8 commits intodevelopmentfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces an is_trained property across multiple model implementations and updates corresponding tests to assert the trained state of the models.
- Adds assertions in tests for RandomForest, GP, and MCMC GP to verify is_trained before and after training
- Modifies the training methods in RandomForest, Multi-objective models, Gaussian process (MCMC based), and AbstractModel to update the _is_trained flag
- Updates the CHANGELOG to document the new trained property
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/test_model/test_rf.py | Added assertions for is_trained in RandomForest tests |
| tests/test_model/test_gp_mcmc.py | Added assertions for is_trained in GP_MCMC tests |
| tests/test_model/test_gp.py | Added a dedicated test for is_trained in GP model |
| smac/model/random_forest/random_forest.py | Sets _is_trained to True after fitting the forest |
| smac/model/multi_objective_model.py | Implements a read-only is_trained property using all() |
| smac/model/gaussian_process/mcmc_gaussian_process.py | Changes _is_trained assignment to depend on all model flags |
| smac/model/abstract_model.py | Initializes _is_trained to False and exposes is_trained |
| CHANGELOG.md | Documents the new is_trained property feature |
Safeguard against the unlikely event that no model is in the MCMCGP, Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR introduces an is_trained property to indicate whether models have been trained. Key changes include:
- Adding tests in models’ test files (rf, gp, gp_mcmc) to verify the state of is_trained before and after training.
- Updating model training methods in random_forest, multi_objective_model, mcmc_gaussian_process, and abstract_model to appropriately set the is_trained flag.
- Documenting the new property in the CHANGELOG.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_model/test_rf.py | Added assertions for is_trained before and after training of RandomForest model |
| tests/test_model/test_gp_mcmc.py | Added assertions for is_trained for the GP MCMC model training |
| tests/test_model/test_gp.py | Added a new test to validate the is_trained property in the GP model |
| smac/model/random_forest/random_forest.py | Sets the is_trained flag to True after fitting the random forest model |
| smac/model/multi_objective_model.py | Introduces is_trained property and sets it based on the training state of internal models |
| smac/model/gaussian_process/mcmc_gaussian_process.py | Updates is_trained flag setting with a condition based on the presence of internal models |
| smac/model/abstract_model.py | Initializes the is_trained flag and exposes it via a property |
| CHANGELOG.md | Updates the changelog to include the new is_trained feature |
benjamc
left a comment
There was a problem hiding this comment.
Can you make sure that the property is_trained is an abstract property, thus required?
If this is difficult with the property type, you can also create an abstractmethod
I have inherited from ABC and made it an abstract property. Now we have a few property getters that will only return if the "_is_trained" attribute is True or False. But future work may want to access that differently, so it kind of makes sense. |
…ting-whether-it-is-trained
#1191