datamol-io / datamol

Molecular Processing Made Easy.
https://docs.datamol.io
Apache License 2.0
462 stars 48 forks source link

Updated logging for _sanifix4.py #172

Closed cwognum closed 1 year ago

cwognum commented 1 year ago

Checklist:


I noticed that in rare occasions, sanitizing a molecule could fail due to seemingly outdated usage of the logging module in _sanifix4.py. I propose to use the RDKit logger instead, because to me personally this code logically falls under RDKit and so that verbosity can be controlled through Datamol's dm.without_rdkit_log() context manager.

Example in which logging could go wrong:

import datamol as dm
import numpy as np
import autosklearn.regression

smi = '[NH4][Pt]([NH4])(Cl)Cl'
mol = dm.to_mol(smi, ordered=True, sanitize=False)
mol = dm.sanitize_mol(mol)  # This will throw the normal warnings as expected

X = np.random.random((100, 10))
y = np.random.random((100, 1))

# AutoML configures the logging module
obj = autosklearn.regression.AutoSklearnRegressor(time_left_for_this_task=30)
obj.fit(X, y)

mol = dm.to_mol(smi, ordered=True, sanitize=False)
mol = dm.sanitize_mol(mol)  # This will now throw a logging error
codecov[bot] commented 1 year ago

Codecov Report

Merging #172 (f7896ef) into main (f226b14) will decrease coverage by 0.12%. The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main     #172      +/-   ##
==========================================
- Coverage   91.20%   91.08%   -0.12%     
==========================================
  Files          47       47              
  Lines        3500     3509       +9     
==========================================
+ Hits         3192     3196       +4     
- Misses        308      313       +5     
Flag Coverage Δ
unittests 91.08% <50.00%> (-0.12%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
datamol/fp.py 84.50% <ø> (ø)
datamol/_sanifix4.py 90.29% <50.00%> (ø)
datamol/_version.py 63.63% <50.00%> (-13.29%) :arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

cwognum commented 1 year ago

@hadim seeing how the codecov fails, I would be happy to add a test but it seems a bit silly to do so for these changes.

hadim commented 1 year ago

Codecov is not blocking here. Thanks!