Skip to content
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

Fix bug for Mol2VecFingerprint #2242

Merged
merged 1 commit into from Oct 30, 2020
Merged

Conversation

nissy-dev
Copy link
Member

@nissy-dev nissy-dev commented Oct 22, 2020

I found the bug about the usage of mol2vec function, so I fixed.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 80.204% when pulling 3ddc610 on nd-02110114:fix-mol2vec into 0fcc6b4 on deepchem:master.

@peastman
Copy link
Contributor

What bug is this fixing?

@nissy-dev
Copy link
Member Author

nissy-dev commented Oct 22, 2020

I made a mistake about how to use sentences2vec method. I should pass the List for the first argument of sentences2vec function, but I didn't. As a result, the return value of sentences2vec was a N×M matrix and I misunderstood that we should apply the some aggregation method like sum or mean to convert the N×M matrix to a M dim vector.

The correct usage is to pass the list to sentences2vec method. By doing so, we can get a vector which we expects, not matrix.

@nissy-dev
Copy link
Member Author

I think this PR is ready for the review

The CI failed by test_weave_fit_simple_infinity_distance, which is not related to this PR.
Recently, the CI is broken by test_weave_fit_simple_infinity_distance

Copy link
Member

@rbharath rbharath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good from my pass!

@nd-02110114 You know this featurizator much better than I do, so feel free to merge in once you feel this is ready :)

@nissy-dev
Copy link
Member Author

Thanks you for checking! I merge in!

@nissy-dev nissy-dev closed this Oct 30, 2020
@nissy-dev nissy-dev reopened this Oct 30, 2020
@nissy-dev nissy-dev merged commit 6ccf705 into deepchem:master Oct 30, 2020
@nissy-dev nissy-dev deleted the fix-mol2vec branch October 30, 2020 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants