[Draft] better handling of shamrock python submodules with pybind11 (instense c++ sorcery)#1806
[Draft] better handling of shamrock python submodules with pybind11 (instense c++ sorcery)#1806tdavidcl wants to merge 10 commits into
Conversation
Add epstein_stopping_time and epstein_supersonic_correction functions to shamphys, implementing the Epstein drag stopping time formula with a supersonic correction factor from Kwok 1975. Assisted-by: claude-code
|
Thanks @tdavidcl for opening this PR! You can do multiple things directly here: Once the workflow completes a message will appear displaying informations related to the run. Also the PR gets automatically reviewed by gemini, you can: |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new submodule registry system and macros for pybind11 module registration, along with a call_lambda utility for static initialization. It also implements Epstein drag physics for spherical dust grains and exposes these calculations to the Python API. Review feedback highlights the need for missing standard library includes in the registry header to ensure it is self-contained, a correction to a file documentation tag, and the removal of redundant or commented-out code in the physics bindings.
| #include <pybind11/pybind11.h> | ||
| #include <map> | ||
| #include <optional> |
There was a problem hiding this comment.
The header is missing several standard library includes required by the registry_t template and the build_all_modules function. Specifically, it needs , <string_view>, , , , , and to be self-contained and avoid compilation errors in different translation units.
| #include <pybind11/pybind11.h> | |
| #include <map> | |
| #include <optional> | |
| #include <pybind11/pybind11.h> | |
| #include <algorithm> | |
| #include <functional> | |
| #include <map> | |
| #include <memory> | |
| #include <optional> | |
| #include <stdexcept> | |
| #include <string> | |
| #include <string_view> | |
| #include <vector> |
| #pragma once | ||
|
|
||
| /** | ||
| * @file print.hpp |
There was a problem hiding this comment.
The @file documentation tag is incorrect. It should refer to call_lambda.hpp instead of print.hpp.
| * @file print.hpp | |
| * @file call_lambda.hpp |
| .def_submodule("phys", "Physics Library"); | ||
| } | ||
|
|
||
| Register_pybind() {} |
There was a problem hiding this comment.
This empty Register_pybind() call is redundant and should be removed to keep the code clean.
| py::module &shamphys_module = shambindings::submodules::modules().get("shamrock.phys"); | ||
|
|
||
| py::module shamphys_module = m.def_submodule("phys", "Physics Library"); | ||
| // py::module shamphys_module = m.def_submodule("phys", "Physics Library"); |
There was a problem hiding this comment.
Remove this commented-out line to maintain code cleanliness. The new registration system replaces this manual submodule definition.
Workflow reportworkflow report corresponding to commit 8ba7eea Light CI is enabled. This will only run the basic tests and not the full tests. Pre-commit check reportSome failures were detected in base source checks checks. Suggested changesDetailed changes :diff --git a/src/shambindings/include/shambindings/pybindaliases.hpp b/src/shambindings/include/shambindings/pybindaliases.hpp
index 29e24d89..c1147c41 100644
--- a/src/shambindings/include/shambindings/pybindaliases.hpp
+++ b/src/shambindings/include/shambindings/pybindaliases.hpp
@@ -199,7 +199,6 @@ void register_pybind_init_func(fct_sig);
_internal_register_pybind_init( \
__shamrock_unique_name(pybind_), __shamrock_unique_name(pybind_class_obj_), root_module)
-
#define Register_pymodsubmodule_int(path, funcname, lambda_name) \
py::module funcname(); \
shambase::call_lambda lambda_name([]() { \
|
1 participant
No description provided.