Skip to content

Conversation

thomasjpfan
Copy link
Member

Reference Issues/PRs

Continues #18514

Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

LGTM, just a little improvement suggestion (feel free to ignore if you disagree):

@@ -474,7 +474,7 @@ def _get_abs_corr_mat(self, X_filled, tolerance=1e-6):
abs_corr_mat = normalize(abs_corr_mat, norm='l1', axis=0, copy=False)
return abs_corr_mat

def _initial_imputation(self, X):
def _initial_imputation(self, X, in_fit=True):
Copy link
Member

Choose a reason for hiding this comment

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

I would find it safer to set in_fit=False as the default value and only set it to true when called in the fit method instead of having to explicitly set it to False anywhere else. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. PR updated with this change.

@amueller amueller merged commit 5fb02bb into scikit-learn:master Nov 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants