[Bug, Update]: tf.keras.Sequential error, uv, ruff, python 3.10, 3.11, 3.12, 3.13#178
Conversation
27fe28b to
4726eda
Compare
|
Changes:
*1
This failed when self.model is a We just need to keep the self.target_layer and do not need to build a second model (self.multi_head) now so I have deleted the multi_head attribute. |
uv for virtual env managment Compatibility with 3.10 3.11 3.12 3.13 New tox matrix
59e7ecf to
9f029b8
Compare
fe3ec13 to
6b5ddf2
Compare
lucashervier
left a comment
There was a problem hiding this comment.
This is a very substantial PR — great work overall. There are many improvements and it’s clear a lot of effort went into it.
For future changes of this size, it might be beneficial to split them into several smaller PRs. In particular, quite a few diffs appear to be formatting-only changes. While those are useful, mixing them with functional updates makes the review a bit harder, as it becomes more difficult to quickly identify the core changes to focus on. I hope I didn’t miss anything important while going through it.
Overall, the direction looks very good 👍 I left a few questions and comments that I think should be clarified before merging, especially around the parts that modify method signatures or behavior.
I also assumed that the workflows were tested and that the existing tests were run successfully.
Agustin-Picard
left a comment
There was a problem hiding this comment.
Great work, you've implemented a lot of improvements that needed to be done (a long time ago for some of them)!
However, as @lucashervier said, maybe we should have split the PR into 2 or 3, as there were some important changes amidst all the formatting changes. No worries, but just keep it in mind for next time.
I see that you propose a ruff check and ruff format but only fix on PEP violations instead of doing both. We could technically fix both in the pre-commit (or only check in CI). What are your thoughts on this?
In this PR, you also propose changes in the way we perform GradCAM and TCAV, but they haven't been explained in the description. We should try to keep a trace of these changes somewhere as they seem out of the blue.
Finally, we should make sure that the docs build locally before merging. You wouldn't want to be like me and merge to find out you now need to make a second patch PR to fix the documentation ^^
Trying to solve #177
As described in the issue, tf.keras.Sequential don't have the model.input and model.output defined even after a call.
These attributes are instead present in model.inputs[0] and model.output[0]. So I have added an elif condition in xplique.attributions.base.py
BlackBoxExplainer.__init__to use the right attributes in case of a Sequential to build the caching of models.TLDR:
Using id(model.inputs[0]) and id(model.outputs[0]) which contains the symbolic tensors (dtype and shape) of the input and output for a tf.keras.Sequential instead of id(model.input) and id(model.output).