-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Update documentation of BasicMolGANModel #3331
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 for the contribution! In general, better to avoid formatting changes in PRs modifying documentation. Can you remove those and update this PR?
deepchem/models/molgan.py
Outdated
@@ -141,8 +147,10 @@ def create_discriminator(self) -> keras.Model: | |||
1. adjacency tensor, containing bond information | |||
2. nodes tensor, containing atom information | |||
The input vectors need to be in one-hot encoding format. | |||
Use MolGAN featurizer for that purpose. It will be simplified | |||
in the future release. | |||
Use MolGAN featurizer for that purpose. |
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 you remove the formatting changes?
deepchem/models/molgan.py
Outdated
i.e. gan.generators[0](noise_input, training=False, sample_generation=True). | ||
In case of training, not flag is necessary. | ||
For training, flag "sample_generation=True" is not necessary |
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 should be reworded.
For training the model, set `sample_generation=False`
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.
Hi, i've made the suggested changes
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.
Looking better, but a few fixes still needed
deepchem/models/molgan.py
Outdated
The format differs depending on intended use (training or sample generation). | ||
For sample generation use flag, sample_generation=True while calling generator | ||
The format differs depending on intended use (sample generation or training). | ||
For sample generation, flag "sample_generation=True" to be used while calling generator |
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.
The original version uses active voice which is better than passive voice. Can you revert back to active voice?
deepchem/models/molgan.py
Outdated
@@ -142,6 +144,7 @@ def create_discriminator(self) -> keras.Model: | |||
The input vectors need to be in one-hot encoding format. | |||
Use MolGAN featurizer for that purpose. It will be simplified | |||
in the future release. | |||
|
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 you remove this empty line?
deepchem/models/molgan.py
Outdated
i.e. gan.generators[0](noise_input, training=False, sample_generation=True). | ||
In case of training, not flag is necessary. | ||
For training the model, set `sample_generation=False` | ||
|
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 you remove this empty line?
I have made the requested changes |
Changes look fine now. I've set the CI to run. If everything looks fine we can merge in |
Hi Bharath, |
added get_noise_input_shape,get_data_input_shapes functions
This reverts commit 44b820d.
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
Update documentation of BasicMolGANModel
added a line of documentation to better define the functionality of the BasicMolGANModel
Cleaned up the documentation to provide a better understanding of the model to the users
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 deepchem
and check no errorsflake8 <modified file> --count
and check no errorspython -m doctest <modified file>
and check no errors