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

Craft integration #134

Merged
merged 5 commits into from Oct 19, 2023
Merged

Conversation

fredericboisnard
Copy link
Contributor

@fredericboisnard fredericboisnard commented Aug 3, 2023

This PR contains the integration of CRAFT in xplique.

The 4 first commits are related to the source code, the last one is about the documentation.

Base classes (1st patch of this PR: 5bfb885)

xplique/concepts/base.py : introduces base classes that will be used by current Craft classes, and potentially future other concepts methods such as ACE & ICE. In particular it contains the following base classes:

  • BaseConceptExtractor: Base class for concept extraction models. This is the base class for Craft.

Craft classes (2nd patch of the PR: 3327cac)

xplique/concepts/craft.py : this file contains the majority of the code for CRAFT. It contains the following classes:

  • BaseCraft: this class contains the core of all CRAFT operations, it is framework agnostic. Some specific operations for Tensorflow or Pytorch are declided in CraftTf or CraftTorch (see below).
  • BaseCraftManager: this class contains the code for the CRAFT Model manager that offers the possibility to run several CRAFT instances for multiple classes. It is framework agnostic as well, so specific operations for Tensorflow or Pytorch take place in dedicated classes CraftManagerTf and CraftManagerTorch

xplique/concepts/craft_tf.py: contains Tensorflow specific implementations: CraftTf and CraftManagerTf.

xplique/concepts/craft_torch.py: contains Pytorch specific implementations: CraftTorch and CraftManagerTorch.

Tests organization (3rd patch: a5a8055)

There are 3 main tests methods which are adapted for Tensorflow and Pytorch in these files:

  • tests/concepts/test_craft_tf.py
  • tests/concepts/test_craft_torch.py

Here is the list of those tests:

  • test_shape: use CRAFT and to process some simple matrixes of various sizes
  • test_wrong_layers: check that Tensorflow or Pytorch implementation can detect if a wrong type of model is used
  • test_classifier: a more complex test, based on a small classifier (whose weights are beeing loaded during the test) and a small dataset, to verify that basic concepts are correctly extracted.

Documentation (4rth patch: 487d3bb)

The documentation is split among the following files:

  • README.md
  • mkdocs.yml
  • docs/index.md
  • docs/tutorials.md
  • docs/api/concepts.md
  • docs/assets/craft.jpeg

Copy link
Collaborator

@dv-ai dv-ai left a comment

Choose a reason for hiding this comment

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

  1. craft.md: I think it will be good to describe a little bit the method before starting to said what the method shall requires
  2. I think craft package shall support Pytorch model thanks to the pytorch wrapper developed. I think with little effort, it can be done with craft package.
  3. visualization function shall be develop too and a tutorial illustrating the usage of the method on imagenet will be very useful.
  4. I will change the name craft_tf by craft

@fredericboisnard fredericboisnard force-pushed the craft_integration branch 3 times, most recently from 11ae00a to 472f2d3 Compare August 18, 2023 22:09
Copy link
Collaborator

@lucashervier lucashervier left a comment

Choose a reason for hiding this comment

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

Technically it looks wonderful! I mean the code is really neat and quite clear. I added some questions and remarks on those points but I had really a few points. However, there is for now 2 major drawbacks:

  • I think the tests are not appropriate, in the sense that considering the methods at stake they should be more developped
  • It misses documentation for both ACE and ICE + the format of the docstring is not harmonized with the other methods.
    Anyway, it is still an impressive work and the harder is done, congrats for that 🔥

docs/api/concepts/craft.md Show resolved Hide resolved
docs/api/concepts/craft.md Outdated Show resolved Hide resolved
docs/api/concepts/craft.md Outdated Show resolved Hide resolved
docs/api/concepts/craft.md Outdated Show resolved Hide resolved
docs/api/concepts/craft.md Outdated Show resolved Hide resolved
xplique/concepts/ace.py Outdated Show resolved Hide resolved
xplique/concepts/base.py Outdated Show resolved Hide resolved
xplique/concepts/base.py Outdated Show resolved Hide resolved
xplique/concepts/craft_tf.py Show resolved Hide resolved
xplique/plots/craft_visualizations.py Outdated Show resolved Hide resolved
@fredericboisnard fredericboisnard force-pushed the craft_integration branch 3 times, most recently from 6512d24 to 6060b32 Compare September 8, 2023 15:46
@fredericboisnard
Copy link
Contributor Author

Rebased on master

@fredericboisnard fredericboisnard force-pushed the craft_integration branch 2 times, most recently from a88c2b7 to 8b0e844 Compare September 8, 2023 16:10
@fredericboisnard
Copy link
Contributor Author

I repushed because I forgot to add the links to the paper + notebook in craft.md ...

mkdocs.yml Outdated Show resolved Hide resolved
@fredericboisnard fredericboisnard force-pushed the craft_integration branch 4 times, most recently from 7cbb98c to daade88 Compare September 18, 2023 15:09
Copy link
Collaborator

@AntoninPoche AntoninPoche left a comment

Choose a reason for hiding this comment

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

Amazing work to adapt Craft code for it to be ready for packaging. It is nice to have both tensorflow and pytorch versions !

I made several comments but I did not finish my review as I did not read the tutorials yet, I will do it tomorrow. Furthermore, I do not have a deep understanding of the method so my remarks are more high-level.

Finally, it seems that your code did not pass the test workflow. You should use pylint xplique and pytest commands to verify you code will pass the workflow.

docs/api/concepts/craft.md Outdated Show resolved Hide resolved
docs/api/concepts/craft.md Outdated Show resolved Hide resolved
docs/api/concepts/craft.md Show resolved Hide resolved
docs/api/concepts/craft.md Outdated Show resolved Hide resolved
docs/api/concepts/craft.md Outdated Show resolved Hide resolved
xplique/concepts/craft.py Outdated Show resolved Hide resolved
xplique/concepts/craft.py Outdated Show resolved Hide resolved
xplique/concepts/craft.py Outdated Show resolved Hide resolved
xplique/concepts/craft.py Show resolved Hide resolved
xplique/concepts/craft_tf.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@AntoninPoche AntoninPoche left a comment

Choose a reason for hiding this comment

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

Great job again, the plots are really pertinent!

However, the documentation is a bit lacking and I have a few remarks on the tutorials:

  • When you make a command line in a notebook, make sure to add the quiet argument to reduce verbosity (-q for the pip install).
  • The tutorials should be added to the Xplique Google Drive to be with the others.
  • Some plot functions are called for each image separately and thus they plot the result in one column. I think it hinders fluidity.
  • One thing I like from other tutorials is that we quickly get an example of results. Here I would make a cell with the fewest lines possible to obtain the all-in-one plot_image_concepts plot. Then describe the steps as you do now.
  • In fact, I would add a quick general description of the method with a schema at the top as done in other unit tutorials, for example for Rise: https://colab.research.google.com/drive/1icu2b1JGfpTRa-ic8tBSXnqqfuCGW2mO.
  • You do not appear as an Author, are you not the author of the tutorials? (The Author line does not correspond to the papers' authors).
  • You did not put the Xpliqe logo at the top of the tutorials.

Nonetheless, those tutorials show awesome results and how easy it is to get them through the API so great work.

Feel free to come see me if you want to discuss my remarks.

xplique/concepts/craft.py Outdated Show resolved Hide resolved
xplique/concepts/craft.py Outdated Show resolved Hide resolved
xplique/concepts/craft.py Show resolved Hide resolved
docs/api/concepts/craft.md Show resolved Hide resolved
docs/api/concepts/craft.md Show resolved Hide resolved
xplique/concepts/craft.py Outdated Show resolved Hide resolved
@fredericboisnard fredericboisnard force-pushed the craft_integration branch 6 times, most recently from b61335f to 30f08e3 Compare October 5, 2023 14:16
@lucashervier
Copy link
Collaborator

It seems that the tests are failing because you used the tqdm package. 2 options for me: either you think it has a real added-value in which case you should add it to the dependencies (requirements.txt and setup.py) or you simply don't use it. Personnally, I have no opinion.

On the other hands, it also fails for craft torch because with the general test torch is not installed and I think we should keep it that way. You should add the test to the 'tests for torch' job. You can import torch in the related python files the same way as it is done for the Pytorch wrapper so that torch is not a global dependency :)

@fredericboisnard fredericboisnard force-pushed the craft_integration branch 2 times, most recently from 78dc7c0 to 2d57e70 Compare October 17, 2023 17:56
Copy link
Collaborator

@AntoninPoche AntoninPoche left a comment

Choose a reason for hiding this comment

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

Amazing work, the PR is close to being merged!

README.md Outdated Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
docs/index.md Show resolved Hide resolved
docs/api/concepts/craft.md Show resolved Hide resolved
xplique/concepts/craft.py Outdated Show resolved Hide resolved
xplique/concepts/craft.py Show resolved Hide resolved
xplique/concepts/craft.py Show resolved Hide resolved
xplique/concepts/craft.py Outdated Show resolved Hide resolved
xplique/concepts/craft_manager.py Show resolved Hide resolved
tests/concepts/test_craft_tf.py Outdated Show resolved Hide resolved
tests/concepts/test_craft_tf.py Outdated Show resolved Hide resolved
tests/concepts/test_craft_tf.py Outdated Show resolved Hide resolved
tests/concepts/test_craft_torch.py Outdated Show resolved Hide resolved
tests/concepts/test_craft_torch.py Outdated Show resolved Hide resolved
xplique/concepts/craft_manager.py Outdated Show resolved Hide resolved
xplique/concepts/craft_tf.py Outdated Show resolved Hide resolved
xplique/concepts/craft_torch.py Outdated Show resolved Hide resolved
xplique/concepts/craft_torch.py Outdated Show resolved Hide resolved
xplique/concepts/craft_torch.py Outdated Show resolved Hide resolved
@deel-ai deel-ai deleted a comment from lucashervier Oct 18, 2023
Signed-off-by: Frederic Boisnard <frederic.boisnard@irt-saintexupery.com>
@fredericboisnard fredericboisnard force-pushed the craft_integration branch 2 times, most recently from 84ab077 to 3310f92 Compare October 18, 2023 14:48
Copy link
Collaborator

@lucashervier lucashervier left a comment

Choose a reason for hiding this comment

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

Nice work team!

Copy link
Collaborator

@AntoninPoche AntoninPoche left a comment

Choose a reason for hiding this comment

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

Well done!

@fredericboisnard fredericboisnard force-pushed the craft_integration branch 2 times, most recently from e685835 to 1763c5e Compare October 18, 2023 15:17
Signed-off-by: Frederic Boisnard <frederic.boisnard@irt-saintexupery.com>
Signed-off-by: Frederic Boisnard <frederic.boisnard@irt-saintexupery.com>
Signed-off-by: Frederic Boisnard <frederic.boisnard@irt-saintexupery.com>
@lucashervier lucashervier merged commit 816f41f into deel-ai:master Oct 19, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants