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

Importing interface.py requires ASE anyway #95

Closed
pierre-24 opened this issue Jan 10, 2020 · 16 comments · Fixed by #222
Closed

Importing interface.py requires ASE anyway #95

pierre-24 opened this issue Jan 10, 2020 · 16 comments · Fixed by #222
Labels
support Question regarding this project or underlying method
Milestone

Comments

@pierre-24
Copy link

pierre-24 commented Jan 10, 2020

Describe the bug
I'm trying to achieve an interface between gaussian and xtb in order to perform xtb:QM onium calculations. Until recently (since it wasn't possible for me to compile xtb in the past), I was using a custom interface inspired by the previous python file (which does not work with the current version, for this reason), but I would prefer to rely on your implementation of the interface.py file.

I don't really want to have ASE as a dependence for that, since I only need to perform a bridge between the two codes. Not that ASE is a heavy dependence, but it would be weird to require it.

To Reproduce
Steps to reproduce the behaviour:

  1. I add the python directory to PYTHONPATH
  2. In the python code, I use from xtb import interface

The result is, obviously,

Traceback (most recent call last):
  File "/home/pbeaujea/g2xtb_interface/interface.py", line 11, in <module>
    from xtb import interface as xtb_interface
  File "/home/pbeaujea/.install_stda/xtb/python/xtb/__init__.py", line 20, in <module>
    from xtb.calculators import GFN1, GFN2, GFN0
  File "/home/pbeaujea/.install_stda/xtb/python/xtb/calculators.py", line 24, in <module>
    from ase.calculators.calculator import Calculator, all_changes
ImportError: No module named 'ase'

... Since __init__.py is always imported.

Expected behaviour

I get why you included calculators in __init__.py , but in my case, it is problematic: I could always copy/paste interface.py from your repo to mine, but that would not be very elegant (and its probably not the best way due to licencing stuffs).

@pierre-24 pierre-24 added the unconfirmed This report has not yet been confirmed by the developers label Jan 10, 2020
@awvwgk awvwgk added python support Question regarding this project or underlying method and removed unconfirmed This report has not yet been confirmed by the developers labels Jan 10, 2020
@awvwgk
Copy link
Member

awvwgk commented Jan 10, 2020

Probably the best way would be to separate the Python API from the ASE calculator for this, maybe even in separate repositories. We should discuss this.

@pierre-24
Copy link
Author

It depends on what you are trying to achieve with it. For me (and in my case), having a interface.py file in a xtb package is ok, since it is easy to import it this way. The ASE calculator would be an example to show that you can then interface xtb with anything, and would be a little bit "higher level abstraction".

But that's my point of view :)

@awvwgk
Copy link
Member

awvwgk commented Jan 10, 2020

I thought including dependencies would ensure that they are present downstream:

install_requires=['ase'],

Right now ASE ensures that structures are represented in a meaningful way, the Atoms class is by far the most easy and complete way to address this problem. An alternative could be something like QCSchema, which has other drawbacks, like it is missing PBCs (see MolSSI/QCSchema#66). A couple of ndarrays is the least preferable way, at least in my opinion.

BTW, ASE offers a QM:MM functionality and a Gaussian calculator.

@awvwgk
Copy link
Member

awvwgk commented Jan 10, 2020

So at this point it is unlikely that we will drop the ASE dependency, but we might guard the import in the __init__.py in case it is not present. Would that help?

@pierre-24
Copy link
Author

pierre-24 commented Jan 10, 2020 via email

@awvwgk awvwgk added this to the v6.2.3 milestone Jan 10, 2020
@awvwgk
Copy link
Member

awvwgk commented Jan 10, 2020

@pierre-24 wouldn't it be easier to implement the Gaussian input and output format directly in xtb? Looking at your project, you have almost more code dealing with the xtb interface than with the Gaussian interface.

Adding the reader for the input format would require the file extension to be detected correctly in file_figure_out_ftype and a specific reader for such format in xtb/molecule_reader.f90. Writing could occur like for the Turbomole gradient file at

xtb/xtb/program_main.f90

Lines 786 to 787 in ad49de1

call generic_header(iprop,'Property Printout',49,10)
if (lgrad) call tmgrad(mol%n,mol%at,mol%xyz,g,etot)

Anything else?

For maybe obvious reasons I can't implement nor test such interface, but it would still be an unique contributing opportunity, to put it that way ;).

@pierre-24
Copy link
Author

pierre-24 commented Jan 10, 2020 via email

@awvwgk awvwgk modified the milestones: v6.2.3, v6.3.0 Jan 24, 2020
@dgasmith
Copy link

+1 here. It would be great to have python basic ctypes interface without the ASE requirement. Happy to take on the extra formatting onus. If you could ensure the interface.py is at least somewhat stable we could copy that as well.

@awvwgk
Copy link
Member

awvwgk commented Jan 24, 2020

Since I am currently reworking the C-API in #102, I could also look over the Python-API again. There is certainly a way to add optional dependencies for Python modules and than separate the ASE calculator implementation from the basic Python-API.

@dgasmith
Copy link

dgasmith commented Jan 24, 2020

A way to accomplish this would be something like the following:

In the setup.py:

    extras_require={
        "ase": ["ase"]
    }

The xtb/__init__.py would not have import ase so then access would be via:

import xtb.ase
xtb.ase.calculator()

or

from xtb.ase import ase_calculator
ase_calculator()

The ase.py could have something like

try:
    import ase
except ModuleNotFoundError:
    raise MolduleNotFoundError("This submodule requires ASE, please install via 'pip install ase'")

This kind of structure seems fairly standard.

@awvwgk
Copy link
Member

awvwgk commented Jan 24, 2020

Thanks, I'll give this a try after finishing #102.

@awvwgk awvwgk linked a pull request May 22, 2020 that will close this issue
@awvwgk
Copy link
Member

awvwgk commented May 22, 2020

Finally decided that the Python API is not going to a part off this repository anymore. We will keep the ctypes interface for testing purposes, but it shouldn't be used for production.

@pierre-24
Copy link
Author

Just to be clear, you'll rely on a fully C API, then ?

@awvwgk
Copy link
Member

awvwgk commented May 22, 2020

Yes, this repository will only expose a C API. The ctypes API has a lot of shortcomings, in my opinion the easiest way to avoid those is to not have a ctypes API for Python.

There will still be a Python API to xtb, but this is another project for another repository, probably I will use CFFI or SWIG for this purpose.

@pierre-24
Copy link
Author

As long as there is an API, it is fine by me :)

@awvwgk
Copy link
Member

awvwgk commented May 23, 2020

The basic Python API is now successfully migrated to https://github.com/grimme-lab/xtb-python.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
support Question regarding this project or underlying method
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants