Fix RobertaFeaturizer #3476 #3898
Conversation
|
Hey, it seems there are some failing tests (see here). Could you take a look at it? |
Modified the unit tests in |
|
The changes looks good to me and the failures are not related to this PR. I request @rbharath for a second pass. |
deepchem/feat/roberta_tokenizer.py
Outdated
| def __call__(self, *args, **kwargs) -> Dict[str, List[int]]: | ||
| return super().__call__(*args, **kwargs) | ||
| def __call__(self, | ||
| datapoints, |
There was a problem hiding this comment.
Can you add a type annotation here?
There was a problem hiding this comment.
Please see the latest commit
deepchem/feat/roberta_tokenizer.py
Outdated
| def __call__(self, | ||
| datapoints, | ||
| padding: bool = True, | ||
| **kwargs) -> Dict[str, List[int]]: |
There was a problem hiding this comment.
Given we are not directly calling the superclass, let's add a brief docstring here
There was a problem hiding this comment.
Please see the latest commit
| @@ -16,8 +16,7 @@ def test_smiles_call(): | |||
| add_special_tokens=True, | |||
| truncation=True) | |||
| for emb in [embedding, embedding_long]: | |||
| assert 'input_ids' in emb.keys() and 'attention_mask' in emb.keys() | |||
There was a problem hiding this comment.
Why are we removing asserts here?
There was a problem hiding this comment.
Because the __call__ function in RobertaFeaturizer now returns the input_ids only, which are the actual embeddings of the molecules. The reason is that _featurize_shard() inCSVLoader is later called, in which the features List needs the embeddings only but not the attention masks (see here). This is actually why I modified the __call__ in RobertaFeaturizer so that it returns the input_ids only, unlike the __call__ in the inherited HuggingFace's RobertaTokenizerFast, which returns the whole dict (input_ids and attention_mask).
So the asserts are removed since attention_mask are not returned anymore, and the test asserts that the lists of input_ids for all the molecules all have the same length after specifying padding=True.
Description
Fix #3476
Convert
datapointstoListif it is apd.Seriesand addpaddingto pad the embeddings to the same lengthType of change
Please check the option that is related to your PR.
Checklist
yapf -i <modified file>and check no errors (yapf version must be 0.32.0)mypy -p deepchemand check no errorsflake8 <modified file> --countand check no errorspython -m doctest <modified file>and check no errors