-
Notifications
You must be signed in to change notification settings - Fork 10
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
Added Temporal Score functions #72
base: master
Are you sure you want to change the base?
Conversation
…ertSpaceCharacterPerturbations and added test cases
Codecov Report
@@ Coverage Diff @@
## master #72 +/- ##
==========================================
+ Coverage 94.91% 95.27% +0.36%
==========================================
Files 8 9 +1
Lines 334 360 +26
==========================================
+ Hits 317 343 +26
Misses 17 17
Continue to review full report at Codecov.
|
tests/test_temporal_score.py
Outdated
import json | ||
from pathlib import Path | ||
|
||
json_path = Path("decepticonlp/metrics/glove_sample.json") |
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.
Shouldn't the glove file be in tests directory?
|
||
class RankCharacters: | ||
""" | ||
Accepts a feature vector torch tensor and outputs a temporal ranking of characters |
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.
Can we have proper docstrings here?
def temporal_score(self, sentence): | ||
""" | ||
Considering a input sequence x1,x2,...,xn | ||
we calculate T(xi) = F(x1,x2,...,xi) - F(x1,x2,...,xi-1) |
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.
Docstrings in our preferred format needed here too? Including information about params and return values?
|
||
def temportal_tail_score(self, sentence): | ||
""" | ||
Considering a input sequence x1,x2,...,xn |
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.
Docstring needed
|
||
def combined_score(self, sentence, lambda_): | ||
""" | ||
Computes combined temporal_score, temportal_tail_score |
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.
Proper docstrings needed
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.
I'll add all of the docstrings
import torch.nn as nn | ||
|
||
|
||
class RankCharacters: |
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.
Is it ranking characters or words?
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.
My bad, this is rank characters. Before this I did it on a character level but later changed it to a word level, forgot to change the class name :P. I'll change this rn.
inputs = sentence.reshape(len(sentence), 1, -1) | ||
model = nn.RNN(inputs.shape[2], 1) | ||
with torch.no_grad(): | ||
_, pred = model(inputs) |
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.
Shouldn't the model be pre-trained on the task? I mean user should have an option to use his own models to test this
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.
@rajaswa , this is the temporal scorer, it selects words based on highest decrease in rnn outputs.
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.
- I am aware of that. But monitoring output from an untrained RNN doesn't make sense at all
- This should be usable by all models working with text sequences
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.
Cool I'll make the changes in that way then !
Kudos, SonarCloud Quality Gate passed! 0 Bugs |
No description provided.