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

Reorganization of some top level modules #2357

Merged

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Dec 18, 2018

Fixes #2311

There are a few top-level modules whose purpose was not fully clear and
partially as well as partially overlapping, which caused the content to
be quite unorganized and scattered. Here we reorganize these modules
after having clearly defined their purpose:

  • aiida.common: for generic utility functions and data structures
  • aiida.manage: for code that manages an AiiDA instance
  • aiida.tools: for code that interfaces with third-party libraries

The module aiida.control has been removed. Code that interacted with
internal components have been moved into cmdline or manage where
applicable. The aiida.control.postgres has been moved to the
aiida.manage.external module which is reserved for code that have to
interact with system wide components, such as the postgres database.

The module aiida.utils has been removed. Generic functionality has
been placed in aiida.common and more specific functionality has been
placed in the appropriate sub folders.

@sphuber
Copy link
Contributor Author

sphuber commented Dec 18, 2018

The only two things not done yet, are the aiida.common.utils.Prettifier class and its related functions, and the aiida.common.orbital module. The latter, together with OrbitalData and ProjectionData are too specific for aiida-core and should be probably be moved to aiida-materials-science plugin. We could do move it to aiida-quantumespresso for the time being, but I am not sure if other plugins also use it.

@coveralls
Copy link

Coverage Status

Coverage increased (+7.4%) to 68.676% when pulling ad86ad3ae546ed01abdce6a05ef30e8af4d2fd4d on sphuber:fix_2311_reorganization_modules into 14ecf15 on aiidateam:provenance_redesign.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+7.4%) to 68.676% when pulling ad86ad3ae546ed01abdce6a05ef30e8af4d2fd4d on sphuber:fix_2311_reorganization_modules into 14ecf15 on aiidateam:provenance_redesign.

@coveralls
Copy link

coveralls commented Dec 18, 2018

Coverage Status

Coverage increased (+7.4%) to 68.711% when pulling 8249edd on sphuber:fix_2311_reorganization_modules into 14ecf15 on aiidateam:provenance_redesign.

@sphuber sphuber force-pushed the fix_2311_reorganization_modules branch 4 times, most recently from 4099613 to 52cbb1a Compare December 18, 2018 15:37
@ltalirz
Copy link
Member

ltalirz commented Dec 18, 2018

Wow, thanks a lot @sphuber !
I'll look through this tomorrow, ok?

@giovannipizzi
Copy link
Member

Thanks a lot @sphuber !!
For aiida.common.orbital, I don't think we should move it to aiida-quantumespresso - it was implemented there on purpose to be generic, and it's also used by at least aiida-wannier90.

I would suggest to either keep it there or move to to a folder that we specifically use for data extensions (I don't know if it really belongs to tools - we could still have a subfolder in some aiida.xxx package to keep these things) until we decide if we want to open this repository for materials science. Or just keep it there for now and put a comment in the issue for the materials science repo for data (I've the feeling there's already an open issue).

For the prettifier, it's used by the kpoints/bands exporters, but it could still be general - also in this case maybe it's ok to keep it where it is?

For the rest, I let @ltalirz accept it - let me know if you also want me to give a look. Is there already a wiki page or somethings similar explaining what the folders are for?

@sphuber
Copy link
Contributor Author

sphuber commented Dec 19, 2018

@giovannipizzi agreed on the OrbitalData and ProjectionData, but is the aiida.common.orbital module really necessary in aiida-core. As far as I can tell it only defines RealHydrogenOrbital as a subclass of Orbital, but it is not used anywhere. Is this used in wannier90? I see that aiida-quantumespresso uses these classes in the projwfc parser

@sphuber sphuber force-pushed the fix_2311_reorganization_modules branch 2 times, most recently from 05047a7 to 3123679 Compare December 19, 2018 17:48
@ltalirz
Copy link
Member

ltalirz commented Dec 19, 2018

Is there already a wiki page or somethings similar explaining what the folders are for?

We now documented the intended content of the folders in the respective __init__.pys.
I think that's already a big step; one could reuse these docstrings via automodule to have them also in the documentation, somewhere near the forward compatibility guarantees #2313

Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case we need to make changes for pre-commit, let me know and I'll re-approve

There are a few top-level modules whose purpose was not fully clear and
partially as well as partially overlapping, which caused the content to
be quite unorganized and scattered. Here we reorganize these modules
after having clearly defined their purpose:

 * aiida.common: for generic utility functions and data structures
 * aiida.manage: for code that manages an AiiDA instance
 * aiida.tools: for code that interfaces with third-party libraries

The module `aiida.control` has been removed. Code that interacted with
internal components have been moved into `cmdline` or `manage` where
applicable. The `aiida.control.postgres` has been moved to the
`aiida.manage.external` module which is reserved for code that have to
interact with system wide components, such as the postgres database.

The module `aiida.utils` has been removed. Generic functionality has
been placed in `aiida.common` and more specific functionality has been
placed in the appropriate sub folders.
@sphuber sphuber force-pushed the fix_2311_reorganization_modules branch from 3123679 to 8249edd Compare December 20, 2018 09:54
@sphuber sphuber merged commit 692eaf3 into aiidateam:provenance_redesign Dec 20, 2018
@sphuber sphuber deleted the fix_2311_reorganization_modules branch January 6, 2019 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants