Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Create Visualizer from a copy of the regressor to prevent updates to original regressor #30

Merged
merged 3 commits into from
Jul 5, 2024

Conversation

SiddhantSadangi
Copy link
Member

create_feature_importance_chart() would instantiate FeatureImportances() on the original regressor and then refit the instantiated object. In some cases (as shown in #29), this can update the coefficients of the original model.

This change instantiates FeatureImportances() on a deepcopy of the original regressor, preventing any changes to the original.

Copy link
Contributor

@AleksanderWWW AleksanderWWW left a comment

Choose a reason for hiding this comment

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

@SiddhantSadangi could you add a test case that would confirm that the regressor is indeed not changed after running the create_feature_importance_chart?

@SiddhantSadangi
Copy link
Member Author

SiddhantSadangi commented Jul 5, 2024

@AleksanderWWW - clubbed it with test_regressor_summary. Lemme know if you want me to create a separate test instead

@AleksanderWWW
Copy link
Contributor

@AleksanderWWW - clubbed it with test_regressor_summary. Lemme know if you want me to create a separate test instead

Up to you, if you feel that it would be worth it then go ahead, but if it covers what needs to be covered already, then leave it as is

Sorry, I wanted to reply and accidentally edited your comment XD

@AleksanderWWW
Copy link
Contributor

Also, check out this https://github.com/scikit-learn/scikit-learn/blob/70fdc843a4b8182d97a3508c1a426acc5e87e980/sklearn/base.py#L40

perhaps we could delegate the copying process to sklearn

@SiddhantSadangi
Copy link
Member Author

sklearn.base.clone() returns an unfitted estimator. It would require us to pass is_fitted=False when instantiating the visualizer. This will require the estimator to be fit as well when the visualizer is fit, increasing runtime.

@SiddhantSadangi SiddhantSadangi merged commit 0f201db into master Jul 5, 2024
11 checks passed
@SiddhantSadangi SiddhantSadangi deleted the ss/stop_updating_model_coef branch July 5, 2024 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants