-
Notifications
You must be signed in to change notification settings - Fork 547
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
V2: fix some docstrings #654
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.
Overall LGTM with some questions for my own information
chemprop/nn/agg.py
Outdated
@@ -47,15 +47,15 @@ def forward(self, H: Tensor, batch: Tensor) -> Tensor: | |||
Parameters | |||
---------- | |||
H : Tensor | |||
A tensor of shape ``V x d`` containing the batched node-level representations of ``n`` | |||
A tensor of shape ``V x d`` containing the batched node-level representations of ``b`` |
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.
Did we define b
anywhere?
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.
It is defined here in message_passing base.py. We could define b
here and probably should, but at the same time we don't define what V
and d
are. Additionally there are many docstrings that use variables without defining them each time. I think it would be better to collect all the variable definitions into a single reference like what some textbooks do, but that is fairly big task.
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.
LGTM! will merge once the tests pass.
Description
As I've read through the code base, I've seen some parts of docstrings that looked incorrect. Some of these are more obvious than others. I've carefully reviewed these changes, but will appreciate another pair of eyes on it before we go to merge.