SauceCat / PDPbox

python partial dependence plot toolbox
http://pdpbox.readthedocs.io/en/latest/
MIT License
840 stars 129 forks source link

pdp_isolate_obj, pdp_interact_obj don't pickle #7

Closed mqk closed 6 years ago

mqk commented 6 years ago

If you try to pickle a pdp_isolate_obj you get a PicklingError:

PicklingError: Can't pickle <type 'instancemethod'>: attribute lookup __builtin__.instancemethod failed

The reason is that currently the model's predict (or predict_proba) method is added as a class member to the object, and pickling instance methods is verboten.

As far as I can tell, there's no reason to add the predict method to class. The pdp_isolate_obj.predict member isn't used anywhere in the code, and it could quite easily be reconstructed from the model, if it were needed. I'd propose to simply remove this member. Happy to submit a PR, if desired.

SauceCat commented 6 years ago

Hi @mqk, Thanks for your advice! Combining with issue #6, I think it might be a good idea that we directly pass the predict method to pdp_isolate function. But in this case, we will also need to pass in the n_classes argument. :grimacing:

mqk commented 6 years ago

To me #6 and #7 are distinct issues. Here I'm just commenting on the pdp_isolate_obj class, which you're just using as a data container (just data, no functionality). I just don't see the need to include the predict method in this container. (Incidentally, a simple dict or a namedtuple might be a better choice for this data container, but opinions differ.)

I'm going to respond to your suggestion about passing predict instead of model to the pdp_isolate function in the other issue, since I think this discussion belongs there.

SauceCat commented 6 years ago

@mqk Agree. Already remove.