Rk45 dev

[o-murphy]

Try it with pytest --engine py_ballisticcalc_exts.rk45_engine:CythonizedRK45IntegrationEngine
Pssing with tolerance>=1e-7

[o-murphy]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a new Cythonized Runge-Kutta-Fehlberg 4(5) integration engine (cythonized_rk45_engine), complete with C++ implementation, Cython bindings, and Python type stubs, and updates the build configuration to include it. The scipy_engine's performance numbers in the documentation have been updated, and its WindSock class has been refactored and optimized to ScipyWindSock, utilizing binary search for improved wind vector lookup performance and direct vector component calculations within the differential equation solver. Review comments highlight several issues with the new RK45 engine, including missing termination conditions in the C++ implementation, an unused time_step parameter, and hardcoded DEFAULT_TIME_STEP and cRK45Tolerance values that should be configurable. Additionally, there are multiple instances of incorrect comments and documentation referring to rk4_engine instead of rk45_engine, a duplicated import in the Cython definition, and a suggestion to add the new RK45 engine to the performance comparison table in the documentation.

I am having trouble creating individual review comments. Click here to see my feedback.

py_ballisticcalc.exts/py_ballisticcalc_exts/src/rk45.cpp (218-223)

This section is commented out and explicitly states "add your actual conditions here". The absence of proper termination conditions means the simulation might run indefinitely or terminate unexpectedly without reaching a meaningful end state. This is a critical functional gap for an integration engine.

Suggestion: Implement the necessary termination conditions (e.g., range limit, minimum velocity, minimum altitude) to ensure the simulation behaves correctly and predictably.

docs/concepts/engines.md (14)

The performance numbers for scipy_engine have been updated. It would be beneficial to add an entry for the new cythonized_rk45_engine in this table, including its speed relative to the baseline and its dependencies, to provide a complete overview of available engines.

py_ballisticcalc.exts/py_ballisticcalc_exts/include/bclibc/rk45.hpp (12)

The @brief description for BCLIBC_integrateRK45 is inaccurate. This function performs the integration, it does not calculate the derivative of velocity. It seems like a copy-paste error from a calculate_dvdt function.

Suggestion: Update the brief to accurately describe the function's purpose.

     * @brief Integrates the projectile trajectory using the Runge-Kutta-Fehlberg 4(5) method.

py_ballisticcalc.exts/py_ballisticcalc_exts/include/bclibc/rk45.hpp (19)

The time_step parameter is declared here but not directly used in the rk45.cpp implementation. Instead, eng.shot.calc_step is used as the current_step and then adapted. This creates confusion about the role of time_step.

Suggestion: Either remove time_step from the signature if it's not intended to be a direct input, or clarify its purpose in the documentation and ensure it influences the initial current_step.

py_ballisticcalc.exts/py_ballisticcalc_exts/rk45_engine.pxd (1)

This comment incorrectly refers to rk4_engine. This file is for rk45_engine.

Suggestion: Update the comment to reflect the correct engine name.

# pxd for rk45_engine to expose CythonizedRK45IntegrationEngine

py_ballisticcalc.exts/py_ballisticcalc_exts/rk45_engine.pxd (7)

The import BCLIBC_BaseTrajDataHandlerInterface is duplicated on line 2 and line 7. This is redundant.

    BCLIBC_Engine,
    CythonizedBaseIntegrationEngine
)

py_ballisticcalc.exts/py_ballisticcalc_exts/rk45_engine.pyi (2-3)

These comments incorrectly refer to rk4_engine and RK4 integration API. This file is for rk45_engine.

Suggestion: Update the comments to reflect the correct engine name and API.

Type stubs for the compiled extension module `py_ballisticcalc_exts.rk45_engine`
to improve IDE completion for the Cythonized RK45 integration API.

py_ballisticcalc.exts/py_ballisticcalc_exts/rk45_engine.pyi (14)

This comment incorrectly states that the class constant is specific to the RK4 engine. It should refer to RK45.

Suggestion: Update the comment to reflect the correct engine.

    # Class constant specific to RK45 engine

py_ballisticcalc.exts/py_ballisticcalc_exts/rk45_engine.pyi (19)

This comment incorrectly refers to the RK4 engine. It should refer to RK45.

Suggestion: Update the comment to reflect the correct engine.

        C/C++-level initializer for the RK45 engine.

py_ballisticcalc.exts/py_ballisticcalc_exts/rk45_engine.pyx (20)

The DEFAULT_TIME_STEP is a magic number. It would be better to make this configurable through the engine's configuration dictionary, similar to how cMinimumVelocity or cStepMultiplier are handled in BaseEngineConfig.

Suggestion: Define DEFAULT_TIME_STEP as a configurable parameter in the engine's config, allowing users to adjust it for different precision/performance needs.

py_ballisticcalc.exts/py_ballisticcalc_exts/src/rk45.cpp (21)

The cRK45Tolerance is a magic number. This tolerance value is crucial for the adaptive step size control and should ideally be configurable through the engine's configuration, rather than being hardcoded.

Suggestion: Expose cRK45Tolerance as a configurable parameter in the engine's configuration, allowing users to fine-tune the precision of the RK45 integration.

[gemini-code-assist]

Code Review

This pull request introduces a new RK45 integration engine, including the C++ implementation, Cython bindings, and necessary setup file modifications. The implementation of the adaptive step-size Runge-Kutta-Fehlberg method is solid, but there are several areas for improvement. My review focuses on addressing copy-paste errors in the new files, improving code clarity by removing unused code and parameters, making the integration tolerance configurable, and correcting misleading documentation. These changes will enhance the maintainability and usability of the new engine.

[gemini-code-assist]

The integration tolerance cRK45Tolerance is hardcoded to 1e-6. The pull request description mentions passing tests with tolerance>=1e-7, suggesting a need for different tolerance values. It would be more flexible to make this configurable, for example by adding a rk45_tolerance field to the BCLIBC_Config struct and using it in the integration function.

[gemini-code-assist]

This comment appears to be a copy-paste from the rk4_engine's .pxd file. It should be updated to refer to rk45_engine.

# pxd for rk45_engine to expose CythonizedRK45IntegrationEngine

[gemini-code-assist]

BCLIBC_BaseTrajDataHandlerInterface is imported twice, once from traj_data and again from base_engine. The redundant import from base_engine should be removed.

from py_ballisticcalc_exts.base_engine cimport (
    BCLIBC_Engine,
    CythonizedBaseIntegrationEngine,
)

[gemini-code-assist]

The velocity variable is declared, assigned values on lines 85 and 198, but is never read. It appears to be dead code and can be removed to improve clarity. The velocity_vector is used for all calculations.

[gemini-code-assist]

Using a leading underscore for a local variable name like _dir_vector is unconventional in C++ and generally reserved for class members. Consider renaming it to dir_vector for better style consistency. You will also need to update its usage on line 95.

        BCLIBC_V3dT dir_vector;

[gemini-code-assist]

The condition || error_e < 1e-12 is redundant because tolerance is 1e-6. Any value of error_e less than 1e-12 will also be less than tolerance. While this might be a safeguard for very small tolerance values, it currently adds complexity. Consider removing it for clarity. The check on line 202 already handles the case where error_e is very small.

                if (error_e <= tolerance)