isayevlab / Auto3D_pkg

Auto3D generates low-energy conformers from SMILES/SDF
MIT License
148 stars 34 forks source link

Add entry point for CLI in auto3D.py #77

Closed j3mdamas closed 6 months ago

j3mdamas commented 6 months ago

Hi,

I think that the current usage of the CLI entry point could be improved using python package entry points.

Usage would change from:

cd <replace with your path_folder_with_auto3D.py>
python auto3D.py "example/files/smiles.smi" --k=1

to

auto3D my-smiles-not-from-code.smi --k=1

which is much better if installing from PyPI and not cloning the source.

I can make a PR for it, just gauging your interest first.

LiuCMU commented 6 months ago

Hey @j3mdamas , thanks for checking with us! I think this will be a great improvement. I appreciate it if you could help us improve and make a PR for this. Many aspects of Auto3D can be improved, and we do love to hear back from the community. Please make sure you pass the unit tests before the PR. I will review ASAP. Thanks!

j3mdamas commented 6 months ago

Just made a PR.

I ran the tests locally:

===================================================== test session starts =====================================================
platform linux -- Python 3.8.18, pytest-8.1.1, pluggy-1.4.0
rootdir: [REDACTED]/Auto3D_pkg
configfile: pyproject.toml
collected 33 items

test_SPE.py ...                                                                                                         [  9%]
test_auto3D.py ...sssss.ssss....                                                                                        [ 60%]
test_isomer_engine.py ...                                                                                               [ 69%]
test_tauto.py s.                                                                                                        [ 75%]
test_thermo.py .....                                                                                                    [ 90%]
test_utils.py ...                                                                                                       [100%]

====================================================== warnings summary =======================================================
../../../venv/lib/python3.8/site-packages/rdkit/Chem/MolStandardize/__init__.py:20
  [REDACTED]/venv/lib/python3.8/site-packages/rdkit/Chem/MolStandardize/__init__.py:20: DeprecationWarning: The module rdkit.Chem.MolStandardize.standardize is deprecated and will be removed in the next release.
    from .standardize import (Standardizer, canonicalize_tautomer_smiles,

../../../venv/lib/python3.8/site-packages/rdkit/Chem/MolStandardize/standardize.py:23
  [REDACTED]/venv/lib/python3.8/site-packages/rdkit/Chem/MolStandardize/standardize.py:23: DeprecationWarning: The module rdkit.Chem.MolStandardize.charge is deprecated and will be removed in the next release.
    from .charge import ACID_BASE_PAIRS, CHARGE_CORRECTIONS, Reionizer, Uncharger

../../../venv/lib/python3.8/site-packages/rdkit/Chem/MolStandardize/standardize.py:24
  [REDACTED]/venv/lib/python3.8/site-packages/rdkit/Chem/MolStandardize/standardize.py:24: DeprecationWarning: The module rdkit.Chem.MolStandardize.fragment is deprecated and will be removed in the next release.
    from .fragment import PREFER_ORGANIC, FragmentRemover, LargestFragmentChooser

../../../venv/lib/python3.8/site-packages/rdkit/Chem/MolStandardize/standardize.py:25
  [REDACTED]/venv/lib/python3.8/site-packages/rdkit/Chem/MolStandardize/standardize.py:25: DeprecationWarning: The module rdkit.Chem.MolStandardize.metal is deprecated and will be removed in the next release.
    from .metal import MetalDisconnector

../../../venv/lib/python3.8/site-packages/rdkit/Chem/MolStandardize/standardize.py:26
  [REDACTED]/venv/lib/python3.8/site-packages/rdkit/Chem/MolStandardize/standardize.py:26: DeprecationWarning: The module rdkit.Chem.MolStandardize.normalize is deprecated and will be removed in the next release.
    from .normalize import MAX_RESTARTS, NORMALIZATIONS, Normalizer

../../../venv/lib/python3.8/site-packages/rdkit/Chem/MolStandardize/standardize.py:27
  [REDACTED]/venv/lib/python3.8/site-packages/rdkit/Chem/MolStandardize/standardize.py:27: DeprecationWarning: The module rdkit.Chem.MolStandardize.tautomer is deprecated and will be removed in the next release.
    from .tautomer import (MAX_TAUTOMERS, TAUTOMER_SCORES, TAUTOMER_TRANSFORMS, TautomerCanonicalizer,

../../../venv/lib/python3.8/site-packages/rdkit/Chem/MolStandardize/__init__.py:22
  [REDACTED]/venv/lib/python3.8/site-packages/rdkit/Chem/MolStandardize/__init__.py:22: DeprecationWarning: The module rdkit.Chem.MolStandardize.validate is deprecated and will be removed in the next release.
    from .validate import Validator, validate_smiles

../../../venv/lib/python3.8/site-packages/rdkit/Chem/MolStandardize/validate.py:24
  [REDACTED]/venv/lib/python3.8/site-packages/rdkit/Chem/MolStandardize/validate.py:24: DeprecationWarning: The module rdkit.Chem.MolStandardize.validations is deprecated and will be removed in the next release.
    from .validations import VALIDATIONS

tests/test_SPE.py::test_calc_spe_ani2xt
tests/test_thermo.py::test_opt_geometry2
  [REDACTED]/venv/lib/python3.8/site-packages/torchani/utils.py:158: UserWarning: To copy construct from a tensor, it is recommended to use sourceTensor.clone().detach() or sourceTensor.clone().detach().requires_grad_(True), rather than torch.tensor(sourceTensor).
    self_energies = torch.tensor(self_energies, dtype=torch.double)

tests/test_auto3D.py::test_auto3D_sdf_rdkit_aimnet
tests/test_auto3D.py::test_auto3D_sdf_rdkit_ani2x
tests/test_auto3D.py::test_auto3D_sdf_rdkit_ani2xt
  [REDACTED]/venv/lib/python3.8/site-packages/Auto3D/utils.py:206: UserWarning: Enumerating stereocenters of an SDF file could change the conformers of the input file. Please use enumerate_isomer=False.
    warnings.warn(msg, UserWarning)

tests/test_thermo.py::test_calc_thermo_aimnet
tests/test_thermo.py::test_calc_thermo_aimnet
tests/test_thermo.py::test_vib_hessian
  [REDACTED]/venv/lib/python3.8/site-packages/ase/utils/__init__.py:62: DeprecationWarning: Please use atoms.calc = calc
    warnings.warn(warning)

tests/test_thermo.py::test_calc_thermo_aimnet
  [REDACTED]/venv/lib/python3.8/site-packages/ase/utils/__init__.py:62: DeprecationWarning: Please use atoms.calc
    warnings.warn(warning)

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
================================== 23 passed, 10 skipped, 17 warnings in 1245.24s (0:20:45) ===================================

I kept the PR straight to the point, and also tried to contribute to the documentation.

Is there a reason why pandas is not on the requirements? I found myself having to install it in addition.

LiuCMU commented 6 months ago

Thanks! I see the PR and will work on it soon.

As for pandas, I guess it was because anaconda installed pandas when creating an environment for me. Since pandas needs to be installed additionally, we could totally add pandas to the requirements.

j3mdamas commented 6 months ago

Yes, I always use a new environment without any package installed. Not a big deal for me, I can manage that from my side, just thought I should mention it.

LiuCMU commented 6 months ago

While I was checking the installation.yml file, I noticed that pandas is specified as part of the dependencies:

https://github.com/isayevlab/Auto3D_pkg/blob/3e462e0da7c5ce1f9004e1d23a91447b4ea41500/installation.yml#L8-L14

Did you create your conda environment using the installation.yml file? I though the dependencies should be installed if the environment is created via conda env create --file installation.yml --name auto3D

j3mdamas commented 6 months ago

Hi @LiuCMU,

You're correct. I must confess I skimmed through it and missed the conda environment. That's mostly because I keep my environments conda-independent.

But since you asked, why not putting numpy, pandas, torch, and rdkit in the dependencies of the package: https://github.com/isayevlab/Auto3D_pkg/blob/752b54378a0aca4a116e27e8d3c0cbee31d43bd1/setup.cfg#L24-L28 ?

I know that for torch it can be more complicated, but it can be done.

The same is true for the optional dependencies like ase, torchani, and even openeye toolkit, which can be added together or individually through the python packaging tooling. Since they are optional, the user can choose not to install them with the package, and use something else instead.

LiuCMU commented 6 months ago

Thanks for the comments!

Honestly, I don't have good excuses. Traditionally, our lab manages python packages with conda, so I kept the dependencies in the installation.yml file so that a conda environment can be set up easily. However, I realized that it is easier to install and manage auto3d with PyPi over the development process. As a result, the dependencies are split in the installation.yml and setup.cfg. The most common packages that I assume Conda users have are put in the installation.yml, and the less common and easy-to-install packages are listed in the setup.cfg file.

My concern is: If I put all dependencies in setup.cfg, would it override a conda user's installed packages?

j3mdamas commented 6 months ago

I was not calling you out, I know the popularity of conda :) I just think the simplest solution should win.

As for your concern, a person can create a new "empty" environment in conda, and then just use pip install and manage the Auto3D package and its dependencies. I just tested creating a new env in conda, installing numpy with conda and then with pip, and it tells me: Requirement already satisfied: numpy in ./miniforge3/lib/python3.10/site-packages (1.26.4), so pip inside conda is already aware of the conda packages surrounding it.

I do understand your concerns. An experienced person that wants to do something else than what you have in the documentation will be able to, so there's no issue.

Going back to the topic of the issue, thanks for accepting and merging the PR. I see it's already released on PyPi. Closing.

LiuCMU commented 6 months ago

Thanks for the suggestions regarding entry point and dependencies! They are very helpful. Please let us know if you have additional comments while using auto3d. Have a great weekend!