search

tensorflow / lucid

A collection of infrastructure and tools for research in neural network interpretability.
Apache License 2.0
4.65k stars 655 forks source link

Fix failing ChannelReducer test #221

Closed bmiselis closed 4 years ago

bmiselis commented 4 years ago

It seems that sklearn.decomposition.base.BaseEstimator does not exist anymore. I've replaced it with sklearn.base.BaseEstimator and all the tests seem to pass now.

bmiselis commented 4 years ago

I feel that it could be safer to explicitly specify versions of the packages that are required to run lucid, either in setup.py or by creating a requirements.txt file. Do you think it would be feasible?

colah commented 4 years ago

For backwards compatibility, would it make check for the existence of one BaseEstimator and fall back to the other? Many colab instances still come with the old version of sklearn pre-installed.

CC @michaelpetrov for updating setup/requirements. My main concern is how this will interact with other packages and with platforms like colab that pre-install particular versions of packages.

bmiselis commented 4 years ago

@colah, sounds doable, sure! I have a question: even if colab comes with some sklearn preinstalled, wouldn't it be possible to install appropriate packages in the very first cell from the requirements file?

colah commented 4 years ago

Hi @bmiselis -- I hope you had a great time over the winter break!

I was vaguely concerned there might be further version issues with colab (eg. maybe other packages installed would break if we forced an upgrade / or conflict) but I don't know much about python packaging. If you know more and think this is fine, I'm happy to go with that. :)

bmiselis commented 4 years ago

@colah thanks!

Regarding potential conflicts I've checked that Colab comes with sklearn preinstalled with version 0.22.1 which works with the proposed update without having to fix any package versioning.

It seems to me that adding the line you suggested (if there's no sklearn.decomposition try the other option) and then I'd consider being more explicit with package versions (sounds like another PR, though).

bmiselis commented 4 years ago

By the way, check out the FutureWarning here:

from sklearn.decomposition.base import BaseEstimator
/Users/bmiselis/.virtualenvs/lucid/lib/python3.6/site-packages/sklearn/utils/deprecation.py:144: FutureWarning: The sklearn.decomposition.base module is  deprecated in version 0.22 and will be removed in version 0.24. The corresponding classes / functions should instead be imported from sklearn.decomposition. Anything that cannot be imported from sklearn.decomposition is now part of the private API.
  warnings.warn(message, FutureWarning)

sklearn.base.BaseEstimator will be used in the future but I'll still leave the if statement that @colah suggested.

colah commented 4 years ago

This is great, @bmiselis! Thank you so much for the PR.