search

aiidateam / aiida-wannier90

AiiDA plugin for the Wannier90 code
https://aiida-wannier90.readthedocs.io
Other
9 stars 15 forks source link

Undesirable dependence on ase in utils.py #71

Closed normarivano closed 4 years ago

normarivano commented 4 years ago

When running tests pylint complains due to the uninstalled ase. `aiida_wannier90/utils.py

Line: 10

pylint: import-error / Unable to import 'ase' (col 4)`

Maybe it´s overkill to have a dependence on ase only for the a = structure.get_ase() function. We could implement an xsf and remove ase.

AntimoMarrazzo commented 4 years ago

First, ASE is a fundamental and basic package for AiiDA - so it actually makes a lot of sense to request the dependency, no matter how many times it is explicitly used in W90. I mean try to do anything in AiiDA that involves a crystal structure, 90% you use ASE so... Second, if something works and it is clean code I would not change it for personal aesthetic reason - I mean if you have spare time to devote to the plugin, I would focus on documentation and testing, rather than reinventing the wheel. The line of the code you mention just uses ASE export function to xsf, why do you want to reimplement that?

greschd commented 4 years ago

The cause for the pylint issue is that aiida-core installs ase only when the atomic_tools extra is specified, and aiida-wannier90 doesn't have the dependency specified.

I agree with @AntimoMarrazzo that it's very likely ASE will be installed anyway, so as far as I'm concerned we can just put it in install_requires.