-
Notifications
You must be signed in to change notification settings - Fork 512
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
Add legend for visualize_text #403
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this PR, @heytitle ! I left one small comment!
captum/attr/_utils/visualization.py
Outdated
dom.append('<b>Legend: </b>') | ||
|
||
for value, label in zip( | ||
[-1, 0, 1], ["Negative", "Neutral", "Positive Attribution"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this PR @heytitle ! It feels a little bit that Attribution is related to positive only. Perhaps leaving it just ["Negative", "Neutral", "Positive"]
would be better and the fact that is attribution is intuitive ?
@NarineK thanks for the comment! I haven't thought about that. I thought attribution can be associated to negative and positive. Anyway, I've updated the PR. |
@NarineK anything else we should do here apart from fixing the lint issues? |
Looks great! I'd update the branch. It looks like the branch is out of date :) Thank you :) |
Summary: Disable pytext install in pip builds to avoid segfault Pull Request resolved: pytorch#404 Reviewed By: bilalsal Differential Revision: D21998560 Pulled By: vivekmig fbshipit-source-id: 16664da0541a998f7d9e54cd6edc0ac7705f0619
…available() up to module level (pytorch#402) Summary: Pull Request resolved: pytorch#402 In this diff we attempt to prevent listing of the tests in `test_jit.py` and `test_data_parallel.py` if CUDA is not available by moving the CUDA check up to the module level. This way we don't have to discover these tests and then attempt to run them only for it to trigger this exact code path somewhere down the setup stage for each test case. The main reason why this takes so long is that the computation for `torch.cuda.is_available()` requires the import of the module `torch._C` which we've profiled in the past to take anywhere between 15-20 seconds. To do this for over 1000 tests every single time we test `//pytorch/captum:attributions` is expensive (both timewise and resource wise). --- Reviewed By: vivekmig Differential Revision: D21966198 fbshipit-source-id: 913173879582df562a188be16560ee60e183b084
Summary: Some housekeeping to prepare for future changes. - Separate out React components into individual files - Switch from class based to function based components - Fix accessibility lint To test: Build the frontend code and use the Insights example. Additionally test using notebook. Pull Request resolved: pytorch#400 Differential Revision: D21969379 Pulled By: edward-io fbshipit-source-id: c8632359877df39a373b20bda2a03a3241163275
…ytorch#406) Summary: Pull Request resolved: pytorch#406 Hide features that don't generate meaningful interpretable features, e.g. embeddings from other models. Reviewed By: vivekmig Differential Revision: D22024855 fbshipit-source-id: f45c8fdbcad7161155545c927777d6c16c5b7349
e48226b
to
7a28dea
Compare
There is one failed test due to time exceeded. I'm not sure how I should fix it. Any suggestion? @NarineK Any further comment on this? |
@heytitle , thank you for the PR! Do you mind updating the branch to fix failing CircleCI! |
@NarineK the test didn't succeed because it was timeout. Any suggestion on fixing the issue? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NarineK has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@heytitle , I rerun the tests and it looks like there are some unrelated test errors. We actually fixed them and if you update your branch they should be gone. Do you mind updating the branch with the latest changes ? |
@NarineK I guess it's good to be merged now :) |
Summary: Related to pytorch#401. The PR adds an option for the user to show the legend for `visualize_text`. <img width="643" alt="image" src="https://user-images.githubusercontent.com/1214890/84386814-45d14780-abf2-11ea-8a5d-6445186597b8.png"> Pull Request resolved: pytorch#403 Reviewed By: bilalsal Differential Revision: D22758674 Pulled By: NarineK fbshipit-source-id: e8d3c7b222098a59f249e44b0f503230d1ca177e
Related to #401.
The PR adds an option for the user to show the legend for
visualize_text
.