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

Restructuring of code elements #6

Open
tfrederiksen opened this issue Feb 23, 2018 · 10 comments
Open

Restructuring of code elements #6

tfrederiksen opened this issue Feb 23, 2018 · 10 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@tfrederiksen
Copy link
Owner

tfrederiksen commented Feb 23, 2018

I propose to reorganize the code into a structure like this

  • docs
  • Inelastica/scripts
  • Inelastica/IO
  • Inelastica/core
  • Inelastica/utils
  • Inelastica/F90
  • examples
  • tests
@tfrederiksen tfrederiksen added enhancement New feature or request help wanted Extra attention is needed labels Feb 23, 2018
@brandimarte
Copy link
Collaborator

Maybe move 'tests' inside of 'Inelastica'?

@zerothi
Copy link
Collaborator

zerothi commented Feb 23, 2018

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
Copy link
Owner Author

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

@tfrederiksen
Copy link
Owner Author

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

@zerothi
Copy link
Collaborator

zerothi commented Feb 23, 2018

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

  • Filenames, definitely lower-case, because people do from Inelastica. import Class ;)
    It creates an easy separation between sub-modules and classes.
  • Classes, definitely not ;) keep CamelCase or whichever you prefer. SiestaIO is fine for instance.

@tfrederiksen
Copy link
Owner Author

tfrederiksen commented Feb 23, 2018

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
Copy link
Collaborator

zerothi commented Feb 24, 2018

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
Copy link
Owner Author

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
Copy link
Collaborator

zerothi commented Feb 25, 2018

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
Copy link
Owner Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants