Add constructor functions for model and tokenizer of MonoBERT/T5 #93
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yuxuan-ji
force-pushed
the
simplify-boilerplate-2
branch
from
bd319d3 to
9a99f6a
Compare
ronakice
reviewed
Sep 20, 2020
Comment on lines
+32
to
+33
| self.model = model or self.get_model() | ||
| self.tokenizer = tokenizer or self.get_tokenizer() |
There was a problem hiding this comment.
I'm not sure but is such code standard python practice?
There was a problem hiding this comment.
@ronakice, is your concern about the use of the or operand? I've seen this being used as a shorthand to
self.model = model if model else self.get_model()so I don't think there's any issues there. Or is it more about the PR in general?
ronakice
approved these changes
Oct 17, 2020
|
@yuxuan-ji actually can you resolve these merge conflicts? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is intended to try and resolve the issue raised in #86 (comment) and closes #86
i.e.
I'm proposing we add constructor functions for the model and tokenizer used in MonoBERT/T5 so that the user doesn't need to manually initialize them to the right class
with this change, i'm also removing MonoBERT/T5's ability to take in strings as parameters, as it becomes redundant
thoughts on this change? I think it might be a more sane solution that what I had before