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

Refactor featurizer #2017

Merged
merged 9 commits into from
Jul 16, 2020
Merged

Refactor featurizer #2017

merged 9 commits into from
Jul 16, 2020

Conversation

nissy-dev
Copy link
Member

@nissy-dev nissy-dev commented Jul 16, 2020

What I did

  • Remove old test
  • Remove MolecularWeight feturizer
    • I think this featurizer is not necessary to maintain. And, the name of the file includes MolecularWeight and RDKitDescriptors is basic.py, it is not so good... So, I removed MolecularWeight and, renamed the file.
  • Refactor base Featurizer
    • Base Featurizer basically override only featurize method
  • Remove deepchem.utils.save.log

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 is some nice cleanup @nd-02110114! Swapping over to logging instead of the old log in the remaining spots and renaming featurize_complexes to featurize both are very nice.

I'm not sure whether we should delete MolecularWeightFeaturizer or deprecate it first. It's not a publicly exposed class so it might be fine to delete it.

@@ -6,36 +6,6 @@
from deepchem.feat.base_classes import MolecularFeaturizer


class MolecularWeight(MolecularFeaturizer):
Copy link
Member

Choose a reason for hiding this comment

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

I think this is safe to delete since it wasn't exposed in the public API deepchem/feat/__init__.py. MolecularWeight isn't an useful featurizer, and I don't think anyone was actually using it.

@peastman Do you think we're OK to delete this or would it be better to deprecate it instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we really think it's not useful, I'd be fine with just removing it.

@nissy-dev nissy-dev changed the title Refactor featurizer [WIP] Refactor featurizer Jul 16, 2020
@nissy-dev
Copy link
Member Author

CI failure is related to pymatgen type checking

@nissy-dev nissy-dev changed the title [WIP] Refactor featurizer Refactor featurizer Jul 16, 2020
@rbharath
Copy link
Member

The only travis failures look to be mypy related so going to ahead and merge this in!

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.

3 participants