I have changed the implementation to rely on Descriptastorus. This is what was done in v1.

I will also note that the featurizer works for mols without any heavy atoms

from chemprop.featurizers.molecule import RDKit2DFeaturizer
from rdkit import Chem

mol = Chem.MolFromSmiles('[H][H].[H]')
featurizer = RDKit2DFeaturizer()
featurizer(mol)

Thanks for adding the new featurizer! Can you add unit test and/or CLI test for it?

Elaborating on my comment above:

• this PR has only added the dependency to the pyproject.toml, meaning that the environment.yml-based tests fail because it is missing
• it can be added to environment.yml but not the conda recipe we use to distribute on conda-forge (meaning we would not have conda install chemprop anymore)
• I think we should avoid adding descriptastorus as a required dependency, since the repository is rarely updated and has broken on us in the past

My suggested alternatives:

• vendor just the one function we want
• add our own feature calculator and standardizer

There's also a theoretical question about whether or not using the descriptastorus features is a data leak. My thinking is that the distributions used to rescale the features are parameterized based on some set that I don't know (does anyone?) that could possibly contain overlapping molecules with a testing or validation set, which would shift the distribution of said features to look more like training and thus constitute a data leak.

Ready for another review.

I removed descriptastorus as a dependency (see below for why).

Can you add unit test and/or CLI test for it?

I added a clunky test, open to improvements on it. Mostly I wanted to make sure that the molecule featurizers don't change how they featurize (e.g. RDKit adds another molecule feature to its list), because Chemprop models from the CLI rely on the featurizers not changing.

I also added a note about using descriptastorus to calculate normalized features to the docs.

why remove descriptastorus

@JacksonBurns had some thoughts about this:

it can be added to environment.yml but not the conda recipe we use to distribute on conda-forge (meaning we would not have conda install chemprop anymore)

Turns out descriptastorus is on conda-forge: https://anaconda.org/conda-forge/descriptastorus

I think we should avoid adding descriptastorus as a required dependency, since the repository is rarely updated and has broken on us in the past

I also think that the previous trouble we had with descriptastorus wasn't that bad. The latest version has an error that makes it incompatible with scipy<1.9, but that version of scipy was released two years ago. The simple solution was to update scipy. This error was fixed two months ago on GitHub, but hasn't been released as a new version.

There's also a theoretical question about whether or not using the descriptastorus features is a data leak. My thinking is that the distributions used to rescale the features are parameterized based on some set that I don't know (does anyone?)

I think this is something to consider. The main reason I chose to not have descriptastorus available from the CLI is that I don't know where their scaling came from. Including it in the CLI is essentially a recommendation to use it as it only requires specifying a flag, which I don't feel I can recommend using if I don't know where it came from. Mostly I want to encourage chemprop users to be aware of what information their model is actually using to make predictions. If they are using descriptors, they should hopefully have some idea of what those descriptors are.

A secondary reason I chose to not use descriptastorus is that it is missing a few of the newer molecular features added to RDKit. See the linked issues for details.

Didn't know it was on conda-forge! That changes my opinion, and I would be OK with it from a software perspective.

I'll note too that the incompatible version of scipy cannot even be installed on the versions of Python that Chemprop v2 supports, see: https://docs.scipy.org/doc/scipy/dev/toolchain.html#numpy

Someone else has also just opened an issue asking about the scaling on the descriptastorus repo: bp-kelley/descriptastorus#31 I'm hoping for a satisfactory answer.

I've started a small test PR on the conda recipe to see if the build will work with descriptastorus.

Hello, anyone know the answer about bp-kelley/descriptastorus#31 ?

Hello, anyone know the answer about bp-kelley/descriptastorus#31 ?

No, we are also curious.

It seems we are going to add Descriptastorus as a dependency. Could you modify the pyproject.toml and environment.yml accordingly?

I've started a small test PR on the conda recipe to see if the build will work with descriptastorus.

The linked PR on the conda recipe confirmed that the build will work.

@shihchengli Am I missing any changes you'd like to see in this PR before merging?

That's a good point. I think we are okay requiring descriptastorus because it available on PyPI and Conda. Let's see if the tests work when it is required. Current users will need to install descriptastorus themselves when they pull in these changes, but that shouldn't be too hard?

Agreed, should be straightforward. Approving!