Added Weave class and WeaveModel class #3529
Conversation
NimishaDey
force-pushed
the
weavemodel-torch
branch
from
3ab66ac
to
3c9a397
Compare
| pad_batches=pad_batches): | ||
| if y_b is not None: | ||
| if self.model.mode == 'classification': | ||
| y_b = to_one_hot(y_b.flatten(), |
There was a problem hiding this comment.
separate handling of labels in the data with in the default generator may not be required
There was a problem hiding this comment.
default_generator function gets called internally for Numpy Dataset when model.fit() is run. And to keep it close to tensorflow implementation I thought it would be best to keep the default generator function.
| n_tasks = self.n_tasks | ||
| if self.mode == 'classification': | ||
| n_classes = self.n_classes | ||
| self.layer_2 = nn.LazyLinear(n_tasks * n_classes) |
There was a problem hiding this comment.
@rbharath will it be fine to use lazy linear over default nn.linear here. The usage here is based on the fact that input size is not known, but it may be possible to get that.
| if weight_decay_penalty != 0.0: | ||
| weights = [layer.weight for layer in self.model.layers2] | ||
| if weight_decay_penalty_type == 'l1': | ||
| regularization_loss = lambda: weight_decay_penalty * torch.sum( # noqa: E731 |
There was a problem hiding this comment.
please check again as we generally don't use #noqa to fix lint issue
There was a problem hiding this comment.
#noqa has to be used for lambda and since regularization_loss is a callable type I think it's required.
| @@ -172,7 +171,8 @@ def __init__( | |||
| ] | |||
| self.batch_normalize: bool = batch_normalize | |||
| self.n_weave: int = n_weave | |||
| torch.manual_seed(21) | |||
|
|
|||
| # torch.manual_seed(21) | |||
There was a problem hiding this comment.
I think you can remove this comment
There was a problem hiding this comment.
I have readded the seed statement because otherwise the reload test is failing.
There was a problem hiding this comment.
Is it fine to add the seed statement here?
| @@ -227,10 +227,15 @@ def __init__( | |||
|
|
|||
| if n_layers > 0: | |||
| self.layers2: nn.ModuleList = nn.ModuleList() | |||
| in_size = 1408 | |||
There was a problem hiding this comment.
is input size always fixed?
There was a problem hiding this comment.
No it actually depends on n_graph_feat. So I have changed this statement in terms of n_graph_feat.
| def test_weave_singletask_classification_overfit(): | ||
| """Test weave model overfits tiny data.""" | ||
| # np.random.seed(123) | ||
| # torch.manual_seed(123) |
There was a problem hiding this comment.
I think seed should be turned on for this test.
|
|
||
| # Eval model on train | ||
| scores = model.evaluate(dataset, [classification_metric]) | ||
|
|
There was a problem hiding this comment.
mention a comment here suggesting to inspect model in future to understand low score
There was a problem hiding this comment.
Okay I will do that. The unit test for tensorflow code uses the same value though.
| else: | ||
| self.layer_2 = nn.Linear(fully_connected_layer_sizes[1], n_tasks) | ||
|
|
||
| def forward(self, inputs: OneOrMany[torch.Tensor]) -> List[torch.Tensor]: |
NimishaDey
force-pushed
the
weavemodel-torch
branch
from
c151463
to
8fee49e
Compare
| Parameters | ||
| ---------- | ||
| inputs: OneOrMany[torch.Tensor] | ||
| Should contain 5 tensors [atom_features, pair_features, pair_split, atom_split, atom_to_pair] |
There was a problem hiding this comment.
Formatting here is a little off
Description
Added Weave class and WeaveModel class.
Type 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