-
Notifications
You must be signed in to change notification settings - Fork 33
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
TudatPy structure #159
Open
alfonsoSR
wants to merge
56
commits into
develop
Choose a base branch
from
submodules
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
TudatPy structure #159
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Splitted tudatpy kernel into submodules - Replaced YACMA with standard method to find python (CMake) - Introduced standard src structure for future integration of stubs
- Moved sbdb and horizons interface from tudatpy/data to numerical simulation to avoid circular imports - Fixed typing issues in python scripts: data, util - A lot of functionality still relies on tudatpy/io, which is currently deprecated, so I moved it to tudatpy/data.
- Defined a CMake-based installation configuration based on the old version of tudatpy as an attempt to fix installation issues on azure. - Replaced stub-only package with in-tree stubs, which seem to work better with current code editors.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issues addressed by this PR
Critical
__init__.py
files: The autogeneration of__init__.py
is problematic for modules including both C++ and Python functionality, adds complexity to the build process and makes it difficult for new developers to understand how to contribute code.expose_X.cpp
undertudatpy/X
rather thantudatpy/kernel/X
as the former also contains the__init__.py
file and, potentially, python-native functionality). The presence of_import_all_kernel_modules.py
files and the fact that they are autogenerated makes it difficult to understand where to contribute code or where to find the source code of the function you are using.Non-critical
tudatpy/__init__.py
). Migrating to a src layout (i.e.tudatpy/src/tudatpy/__init__.py
) would allow for the creation of a stub-only package at no cost and significantly simplify a potential unification of tudat and tudatpy into a standalone repository (e.g. adding tudat, spice, sofa... totudatpy/src
). Both flat and src layouts are standard and accepted, but I consider the latter more suitable for this kind of project.Solutions
Import time
I modified the build process to generate one shared library per submodule (i.e. per
expose_X.cpp
file). This results into import times being proportional to the size of the imported modules (e.g. import time will still be relatively high for a program loadingnumerical_simulation
, but not for one loadingmath
orastro
, since those modules are much smaller), which could be reduced by splitting big submodules per functionality (e.g.numerical_simulation
->dynamics
&estimation
orestimation_setup.observation
being splitted into submodules) or optimizing the compilation process.Organization of source code
I moved each
expose_X.cpp
script to its submodule's directory (as suggested above) and modified the build process such that the associated kernel is saved in the same directory. This allows for the replacement of the autogenerated__init__.py
and_import_all_kernel_modules.py
files with a standard__init__.py
scripts to be maintained by developers. Thus, a hybrid submodule (one including exposed and python-native functionality) X would have the following structure:and the
__init__.py
file would look as follows:Under the assumption that all the functionality defined in
expose_X.cpp
should belong to the API, the star import fromexpose_X
prevents developers from having to update the__init__.py
files when they expose a new function, effectively replacing autogeneration, while the manual imports from the python scripts allow them to keep "private" functions and helpers out of tudatpy's API. The presence of the shared libraries in the source directories is just a personal preference, but moving them into alib
orkernel
directory would not be a problem.NOTE: A positive side effect of having split the kernel is that the header files associated with the exposition scripts are no longer needed.
Docstrings
For each exposed function, I read the content of the docstring from the YAML file, gave it an adequate format and pasted it together with the source code that exposes the function. This eliminates the need for multidoc and the results into docstrings being included by default in all tudatpy distributions. As an example, this is the code snippet used to expose
spice.load_kernel()
:Syntax highlighting
Taking #154 as reference, I included automatic stub generation as part of the build process. Stubs are generated using pybind11-stubgen following the compilation of tudatpy, and stored in a stub-only package with the same structure as the source directory, as described in Packaging type information.
The package is automatically installed together with tudatpy and provides syntax highlighters with information about the available functionality per submodule, function and class signatures, docstrings... This allows for regular autocompletion and greatly simplifies the access to typing and usage information.
Some notes on stub generation
The reason why I opted for pybind11-stubgen instead of mypy's stubgen is that the latter is not compatible with the generation of stub-only packages, is bad at inferring the signature of exposed functions and includes undesired information in the docstrings.
While significantly better than stubgen, pybind11-stubgen includes undesired
from __future__
imports and is not particularly good at generating stubs for__init__.py
files. Thus, while the vast majority of the stub generation process relies on pybind11-stubgen, I wrote some post-processing functions and a parser to generate__init__.pyi
stubs, remove undesired imports and ensure proper indentation within docstrings.Missing
API: This PR is not necessarily compatible with the current way in which the API website is generated. This will be addressed through an additional PR in the near future.
CMake: The current CMake setup relies on deprecated functionality (we require CMake 2.8 when the latest version is 3.30) and non-standard tools such as YACMA. The resulting setup is stiff and difficult to understand, so it should be updated. I attempted this and managed to make it work locally, but not on Azure. More information on [ADD ISSUE]