[MRG] Save sample_weight_arr instead of sample_weight in KernelDensity #13772
Conversation
|
@jnothman @NicolasHug Can be merged |
There was a problem hiding this comment.
This looks correct, though I'm wondering if we're doing the wrong thing to not try make pickles backwards compatible (by making the sample_weight entry in the pickle optional)... Though this is not really due to your PR.
I'd like it if the storing of memory views and sum_weight was refactored into a helper method to avoid duplication in init and setstate.
@jnothman So you mean that if we dont supply sample_weight during fit, we should not be pickling sample_weight_arr? |
|
No, I mean that if there are only 12 elements in the pickle state we can
set the sample_weight to None.
|
@jnothman Wont there always be 12 elements in the pickle? We are pickling sample_weight_arr irrespective of whether we are supplying sample_weight or not |
|
If you were loading a pickle from a previous release of scikit-learn there
wouldn't be. Formally we don't support such pickle compatibility... And
maybe I'm wrong to suggest we should do so here.
|
|
Yeah, just don't worry about that. The user can deal with cross-version
pickle breakage
|
@jnothman Where do you want to do refactoring? |
|
Refactoring between `__init__` and `__setstate__`. There is some
duplication of initialisation there. We would have avoided the present
problem, I believe, if that was more clearly factored out.
|
|
@jnothman @aditya1702 thank you for the fix! do you have an estimate on when it will be released? |
@Dathiou @jnothman I am currently busy with my final semester exams. Will refactor and make the changes after 15th. |
|
I pushed a commit to address some of @jnothman's comments. Let's see if the CI likes it. |
|
But you otherwise approve? I'd like to push out 0.21.1 and it might as well
include this.
|
|
Merged! Thanks @aditya1702 for your contribution. @jnothman I let you take care of the 0.21.1 backport. |
Reference Issues/PRs
Fixes #13692
What does this implement/fix? Explain your changes.
Saves the sample_weight_arr instead of sample_weight. This enables the KernelDensity model object to be dumped into a pickle file. Also adds the relevant test for this.