-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
refactor: add type hints for encoders #3449
refactor: add type hints for encoders #3449
Conversation
Unit Test Results 4 files ±0 4 suites ±0 57m 19s ⏱️ + 1m 24s Results for commit 70fb4fb. ± Comparison against base commit 60f1416. This pull request skips 2 tests.
♻️ This comment has been updated with latest results. |
f694ef5
to
745375a
Compare
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.
Thanks for the change! This type of change is great for code readability.
import torch | ||
|
||
|
||
class EncoderOutputDict(TypedDict, total=False): |
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.
It would be good to add a comment about which encoders return which tensors, or that only sequence encoders return encoder_output_state
and attentions
.
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.
Yeah, your're right! I've added the comments. attentions
are only used by the ViTLegacy Encoder, which is planned to be removed in 0.8. So this return tensor can then be removed too
During implementation I noticed that the bag, set and categorical encoders return simple tensors instead of dicts. Is this intentional, or is there some work to do to make them return dicts as well? (I could take this on) |
Good observation! I'd say it's not intentional and probably came about once there were multiple tensors that needed to be returned from the encoders. CC: @w4nderlust @tgaddair @jimthompson5802 The reason why the model still works with some encoders returning tensor dicts with an You'll notice that the This is definitely a bit confusing -- if making encoder outputs consistent something you would be able to pick up as part of this PR, that would be awesome. |
Thanks for the explanation. I'll give it a try |
e5d6a83
to
0df45d0
Compare
0df45d0
to
f3eeee4
Compare
f3eeee4
to
5595148
Compare
Some integration tests with the explainer functionality are failing. I am not very familiar with the explainer feature. Can you give me some hints on how to fix them? |
Hi @Dennis-Rall, I pushed up a commit that should fix the explanation tests. In Ludwig's implementation of explainability with captum, we provide a list of torch modules that should be used to compute attribution scores for each feature. For embedded feature types, the layers provided to captum are determined by Your PR modified category encoders to return a dictionary of tensors (to make this consistent with the rest of ludwig). So the additional change to fix explainability functionality is to add an additional Hope that was clarifying! |
Yeah, thanks for the explanation and the fix 😄 |
encoder_output
key is present in the returned dict