tfrederiksen / inelastica

Python package for eigenchannels, vibrations and inelastic electron transport based on SIESTA/TranSIESTA DFT
https://tfrederiksen.github.io/inelastica
GNU Lesser General Public License v3.0
33 stars 16 forks source link

Restructuring of code elements #6

Open tfrederiksen opened 6 years ago

tfrederiksen commented 6 years ago

I propose to reorganize the code into a structure like this

brandimarte commented 6 years ago

Maybe move 'tests' inside of 'Inelastica'?

zerothi commented 6 years ago

From my perspective I think having uppercase sub-modules is unnecessary (and error prone).

All is good except IO -> io, but your call in the end. Also, change F90 -> fortran or something less fixated on f90.

tfrederiksen commented 6 years ago

Would you also suggest to go lower-case elsewhere (package name itself, filenames, classes etc)?

tfrederiksen commented 6 years ago

Yes, we can have tests inside the main package directory too

zerothi commented 6 years ago

As for the package name I am not too sure. I think it is a matter of taste. You decide, for Python standard library they use all-lower-case module/submodule etc. https://www.python.org/dev/peps/pep-0008/#id40

tfrederiksen commented 6 years ago

OK, then we stick with Inelastica (uppercase) for the project name, the main package directory, and the script itself. And inelastica (lowercase) for the repository. At least for now.

zerothi commented 6 years ago

With respect to the latest changes! PS. Great job on moving this so quickly.

In Inelastica/__init__.py all sub-packages are from .NEGF import * which may not be exactly what you anticipate it to do? It basically means that all functions enters the Inelastica.* namespace. This may, or may not be your intention.

I.e. a script:

import Inelastica as I

I.GF, I.myHash # function in NEGF.py
I.* # where basically * means *any* function not prefixed with `_`

are all global to the user. Any function defined throughout inelastica without the prefix _ is added to the inelastica name-space.

Just FYI.

tfrederiksen commented 6 years ago

Thanks for your input. No, I guess this is not really what we intend to do... ;)

Which approach would you recommend to fix this? To go through the different modules and add prefixes _ to functions not needed for the user?

zerothi commented 6 years ago

As a starter I would probably not do anything. It is more when going forward it may be nice to think about this. For instance consider whether STM-related, Phonon-related, e-ph-related things should be in separate modules or not? But don't spend too much time on it.

However, there are several ways of going about this.

  1. If you have a bunch of related methods that shouldn't be exposed to the user, put them in a sub-module named _* (this is what I do in sisl, see e.g. sisl._array).
  2. Define __all__ in your files. When doing * imports only the methods/variables mentioned in __all__ will be exposed, by default __all__ are all named variables that does not contain the prefix _.
  3. As you say, if you have a local routine, simply name it _* instead.

All are viable and easy to use solutions. I use a mix of all methods depending on each methods exposure. E.g. if the method is only used in a single module, then it is best to put it there with _* name.

tfrederiksen commented 6 years ago

Great, thanks for the advice. I will try to take this into account when going forward.