Make loading datasets skip over corrupt entries with invalid CMILES

[j-wags]

Todos

• [x] Fix #216
• [x] Add tests
• [ ] ~(optional) Make both backends give the same number of molecules when loading the industry dataset.~ (these changes need to go into the OpenFF Toolkit)
• [x] Emit a warning with a dataset entry is skipped for having an invalid CMILES.
• [x] ~Open issue in OpenFF Toolkit about making RDKitToolkitWrapper be strict about mapped SMILES with implicit Hs~ https://github.com/openforcefield/openff-toolkit/issues/1696
• [x] ~Open issue in OpenFF Toolkit about preventing such SMILES from being created again.~ https://github.com/openforcefield/openff-toolkit/issues/1697
• [x] ~Open issue in qca-dataset-submission about the corrupt entries (not planned for immediate action, but as a place for people to collect requests for the creation of a cleaned dataset)~ https://github.com/openforcefield/qca-dataset-submission/issues/327

Status

• [x] Ready to go

[jthorton]

@j-wags Thanks for sorting this one, I have added a mocked dataset with an invalid entry which should test this case. Thinking about the other dataset types would we want to extend this fix to those as well?

[codecov[bot]]

Codecov Report

Merging #228 (19f3b1b) into main (1538216) will decrease coverage by 0.02%. The diff coverage is 92.30%.

Additional details and impacted files

[j-wags]

Oh my gosh, I tried so hard to make a mocked dataset like that, and now you made it look so easy.

Ok, merging in the changes from main, this looks pretty good. In its current form this keeps the OE backend from crashing on bad entries, but loading the dataset using OE vs. RDKit will yield a different number of molecules. I might make a push to standardize that, but in the longer run I'd kinda rather switch QCSubmit over to an RDKit-only backend to avoid this whole category of problems.

This may be ready to merge as-is. I'll sleep on it. Thanks again for the help @jthorton!

[jthorton]

In its current form this keeps the OE backend from crashing on bad entries, but loading the dataset using OE vs. RDKit will yield a different number of molecules.

Good point should this be an issue on the toolkit that it should raise an error when trying to create a molecule from an incomplete mapped smiles?

I might make a push to standardize that, but in the longer run I'd kinda rather switch QCSubmit over to an RDKit-only backend to avoid this whole category of problems.

That was my original idea however protonation state enumeration is only available via openeye currently and removing that would be an issue for reproducibility :(

[j-wags]

Yeah, after some thought the only pathway to consistently handling the corrupt molecules with both backends is to make changes in RDKitToolkitWrapper in the OFF Toolkit.

I've added some to-dos above - Most of them are bookkeeping and followups, but one thing I'm going to do here is add a warning when a dataset entry is skipped for having an invalid CMILES.

[j-wags]

Thanks for the huge assist @jthorton!