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

Cnn influence example #195

Merged
merged 31 commits into from
Dec 29, 2022
Merged

Cnn influence example #195

merged 31 commits into from
Dec 29, 2022

Conversation

Xuzzo
Copy link
Collaborator

@Xuzzo Xuzzo commented Nov 23, 2022

Description

This PR introduces an example where influence functions are used for CNNs.

Changes

Checklist

  • Wrote Unit tests (if necessary)
  • Updated Documentation (if necessary)
  • Updated Changelog
  • If notebooks were added/changed, added boilerplate cells are tagged with "nbsphinx":"hidden"

@mdbenito mdbenito added documentation Improvements or additions to documentation enhancement New feature or request labels Nov 24, 2022
@Xuzzo Xuzzo self-assigned this Dec 6, 2022
requirements.txt Outdated Show resolved Hide resolved
@Xuzzo Xuzzo marked this pull request as ready for review December 9, 2022 13:51
@Xuzzo
Copy link
Collaborator Author

Xuzzo commented Dec 19, 2022

  • Removed caching of results. Kept the one for the model
  • I have added the difference in score between corrupted and simple scores in the last table
  • Fixed tqdm for model training and influence calculation

@Xuzzo Xuzzo requested a review from AnesBenmerzoug December 19, 2022 13:01
@AnesBenmerzoug
Copy link
Collaborator

@Xuzzo Thanks for your work! It looks better now.

The main thing I noticed is that in the imagenet notebook when you load the dataset it prints information that is useless to the notebook reader:

image

Perhaps we should call datasets.utils.logging.set_verbosity_error() in the load_preprocess_imagenet function.

Copy link
Collaborator

@mdbenito mdbenito left a comment

Choose a reason for hiding this comment

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

I've looked a bit at the supporting code but couldn't check the notebooks yet. Will do asap

notebooks/notebook_support.py Outdated Show resolved Hide resolved
notebooks/notebook_support.py Show resolved Hide resolved
notebooks/notebook_support.py Outdated Show resolved Hide resolved
notebooks/notebook_support.py Outdated Show resolved Hide resolved
notebooks/notebook_support.py Outdated Show resolved Hide resolved
notebooks/notebook_support.py Outdated Show resolved Hide resolved
notebooks/notebook_support.py Outdated Show resolved Hide resolved
notebooks/notebook_support.py Outdated Show resolved Hide resolved
notebooks/notebook_support.py Outdated Show resolved Hide resolved
src/pydvl/influence/conjugate_gradient.py Outdated Show resolved Hide resolved
@Xuzzo
Copy link
Collaborator Author

Xuzzo commented Dec 20, 2022

MR comments should be addressed.

Copy link
Collaborator

@AnesBenmerzoug AnesBenmerzoug left a comment

Choose a reason for hiding this comment

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

I had another quick look at it. It looks good to me and can be merged!

@mdbenito
Copy link
Collaborator

Hi hi,

the supporting code is better now, thanks!

I haven't had time to check it in detail yet, but the notebook:

  • contains several typos, please run it through a spell checker. Some of them are recurring and I have fixed many times in the past, like "data-points". 🙏🏽
  • does not hide cells as instructed in the PR template and CONTRIBUTING.md (why?)
  • Points to a whole book as reference for the main computation. At least one should mention the section or theorem.
  • has a (useful) theory section which ignores the theory section in the review document draft, and is based on a paper which diverges from the usual theory of influence functions.

I understand that it makes sense to be consistent with the paper that is implemented, but a major selling point of the review and of a library like pydvl is that we provide a common framework and notation to understand any paper on influence functions. In particular, it is important to unify notation and be general enough. Koh2017 is a rather sloppy source because it fails to properly define the object that is being approximated and also diverges from the usual definitions. Custom definitions are ok, but we should strive to make the link to what is standard in the literature, I believe.

Now, the stub I wrote over a year ago is itself worse than sloppy, I'm not claiming it's any better (actually I just read it and it's pretty confusing, to put it mildly 😅). But it'd be nice to try to link both. It is ok to remain superficial in the notebook, if it is consistent with the review. Maybe you can start by mentioning the latter, and in the future work to improve that document (we should coordinate, though).

@Xuzzo
Copy link
Collaborator Author

Xuzzo commented Dec 28, 2022

To do, from discussion in #235:

  • add notebook requirements
  • check model saving
  • check dataset ram usage: add comment on downsampling ds in notebook
  • Change InternalDataset to TensorDataset
  • fix pipeline

@mdbenito mdbenito merged commit fd442ec into develop Dec 29, 2022
@mdbenito mdbenito deleted the cnn_influence_example branch December 29, 2022 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
3 participants