-
Notifications
You must be signed in to change notification settings - Fork 6
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
Adding evaluation of TURNA-encoder #74
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.
Thanks a lot for your efforts on fixing the TURNA encoder and its evaluation!
I've left a few minor comments, but other than that, I'm happy to approve this PR!
try: | ||
self.encoder = T5EncoderModel.from_pretrained(pretrained_model_name) | ||
except Exception as e: | ||
pretrained_model_name = config._name_or_path | ||
self.encoder = T5EncoderModel.from_pretrained(pretrained_model_name) | ||
|
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.
Do we really need exception handling here? When does the encoder initialization fail with pretrained_model_name
?
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.
This is really bad code I realize :D but there was a problem with the from_pretrained
method, when it calls the init of T5forSequenceClassification, that argument is overwritten as the config.. I couldn't solve it, so I added this.. If I can find a solution, I will fix it! 👍
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 predict if it's going to fail beforehand and update the pretrained_model_name accordingly? In what scenarios do we use the first from_pretrained method compared to the other one?
try: | ||
return(float(label.strip())) | ||
except: | ||
return 0 | ||
try: | ||
return(float(label)) | ||
except: | ||
return 0 | ||
|
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.
Doesn't float(label.strip())
cover float(label)
already?
Did we check the outputs to see if they contain similarity scores where they cannot immediately cast to float?
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.
When we use conditional generation, the output is a string, so float(label.strip())
works. When we use classification, the output is a number, so the strip()
function naturally gives an error, so I try converting into a float.
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 see. Rather than handling it with exceptions, I suggest checking its type to choose the conversion. It would be much clearer.
In this PR, the evaluation of fine-tuned TURNA-encoder models was implemented. Changes include: