Skip to content

Upgrade propulsion.py to use mpmath for arbitrary precision#21

Merged
lookbusy1344 merged 3 commits intomainfrom
copilot/upgrade-propulsion-to-mpmath
Nov 17, 2025
Merged

Upgrade propulsion.py to use mpmath for arbitrary precision#21
lookbusy1344 merged 3 commits intomainfrom
copilot/upgrade-propulsion-to-mpmath

Conversation

Copy link
Contributor

Copilot AI commented Nov 17, 2025

  • Understand current implementation of propulsion.py and relativity_lib.py
  • Update propulsion.py functions to use mpmath for inputs/outputs with ensure() conversions
  • Import necessary components from relativity_lib
  • Update speed of light constant to use mpmath from relativity_lib
  • Test all four functions with mpmath values
  • Create unit tests for propulsion.py functions (16 test cases)
  • Run tests and verify output matches expected results
  • Run security scan with codeql_checker (0 issues found)
  • Update main section to use rl.seconds_per_year for cleaner code

Summary

Successfully upgraded Python/propulsion.py to use mpmath values following the same pattern as relativity_lib.py:

Changes Made:

  1. Imports: Added mpmath and relativity_lib imports
  2. Type conversions: All inputs converted using rl.ensure()
  3. Constants: Now using rl.c, rl.g, rl.zero, rl.one, rl.seconds_per_year instead of hardcoded floats
  4. Return types: All functions return mpmath numbers
  5. API compatibility: Functions accept both float and mpmath inputs
  6. Default parameters: g parameter defaults to None to use relativity_lib.g

Functions Updated:

  • photon_rocket_accel_time() - Photon rocket acceleration time
  • pion_rocket_accel_time() - Charged-pion rocket acceleration time
  • photon_rocket_fuel_fraction() - Photon rocket fuel fraction
  • pion_rocket_fuel_fraction() - Pion rocket fuel fraction

Testing:

  • Created comprehensive test suite with 16 unit tests
  • All tests pass (100% success rate)
  • Verified backward compatibility with original output
  • Tested with arbitrary precision (50 and 100 decimal places)
  • Security scan: 0 vulnerabilities found
Original prompt

This section details on the original issue you should resolve

<issue_title>Propulsion.py to use mpmath</issue_title>
<issue_description>Upgrade Python/propulsion.py to use mpmath values for input and output, in the same way as relativity_lib.py. Use ensure() for the conversions.</issue_description>

Comments on the Issue (you are @copilot in this section)


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI self-assigned this Nov 17, 2025
Co-authored-by: lookbusy1344 <3680611+lookbusy1344@users.noreply.github.com>
Copilot AI changed the title [WIP] Update propulsion.py to use mpmath for conversions Upgrade propulsion.py to use mpmath for arbitrary precision Nov 17, 2025
Copilot AI requested a review from lookbusy1344 November 17, 2025 22:36
@lookbusy1344
Copy link
Owner

How does this PR look @claude

@claude
Copy link
Contributor

claude bot commented Nov 17, 2025

Claude finished @lookbusy1344's task —— View job


Code Review Complete ✅

I've reviewed the PR that upgrades propulsion.py to use mpmath for arbitrary precision calculations. Overall, this is a well-executed implementation that properly aligns with the relativity_lib.py patterns. Here's my detailed analysis:

Todo List

  • Read repository guidelines (CLAUDE.md)
  • Examine changed files
  • Review code changes and quality
  • Analyze test coverage
  • Provide comprehensive feedback

Strengths ✨

  1. Excellent mpmath integration

    • All inputs properly converted using rl.ensure() (propulsion.py:28-30, 75-77, 119-126, 157-164)
    • Returns mpmath types consistently
    • Uses mpmath mathematical functions (mp.log, mp.exp) instead of math module
  2. Proper constant usage

    • Replaced hardcoded values with rl.c and rl.g from relativity_lib
    • Uses rl.zero and rl.one for comparisons and arithmetic (propulsion.py:43, 92, 132, 169, 175)
    • Maintains precision by using string literals for constants: rl.ensure("0.94") (propulsion.py:86, 166)
  3. Smart API design

    • g parameter defaults to None to use rl.g, with backward compatibility for explicit values
    • Clean pattern: check for None, use library default, otherwise convert (propulsion.py:33-36, 80-83, 122-126, 161-164)
  4. Comprehensive test coverage

    • 16 unit tests covering various scenarios
    • Tests precision maintenance (test_propulsion.py:212-228)
    • Tests type conversion with both float and mpmath inputs
    • Tests edge cases (zero fuel, zero efficiency)
    • Tests mathematical consistency between related functions (test_propulsion.py:175-192)
    • Good use of assertIsInstance(result, mp.mpf) to verify mpmath types
  5. Updated documentation

    • All docstrings updated to reflect mpmath types
    • Clear parameter descriptions
    • Maintains readability in __main__ example with proper float() conversions for display

Issues Found 🔍

Critical Issue: Type Hints Removed

The original code had Python type hints, which have been completely removed:

Before:

def photon_rocket_accel_time(
    fuel_mass: float, dry_mass: float, efficiency: float = 1.0, g: float = 9.80665
) -> float:

After:

def photon_rocket_accel_time(
    fuel_mass, dry_mass, efficiency=1.0, g=None
):

Impact: Loss of static type checking and IDE autocomplete support.

Recommendation: Add type hints that reflect the flexible input/output types. Consider using Union types:

from typing import Union
from mpmath import mpf

def photon_rocket_accel_time(
    fuel_mass: Union[float, mpf], 
    dry_mass: Union[float, mpf], 
    efficiency: Union[float, mpf] = 1.0, 
    g: Union[float, mpf, None] = None
) -> mpf:

Or use relativity_lib's pattern if it defines a type alias for mpmath values.

Minor Issue: Inconsistent Division in main

In the __main__ example (propulsion.py:186), the year calculation chains divisions:

years = t / rl.ensure(60.0) / rl.ensure(60.0) / rl.ensure(24.0) / rl.ensure("365.25")

Suggestion: This could be simplified for readability:

years = t / rl.seconds_per_year

Since relativity_lib.py:54 already defines seconds_per_year = mp.mpf(60 * 60 * 24) * mp.mpf("365.25"), this would be cleaner and avoid repeated ensure() calls.


Code Quality Assessment 📊

Aspect Rating Notes
Correctness ✅ Excellent All mathematical formulas properly converted
Precision ✅ Excellent Proper use of mpmath throughout
Consistency ✅ Excellent Matches relativity_lib patterns perfectly
API Design ✅ Excellent Backward compatible with sensible defaults
Testing ✅ Excellent Comprehensive test coverage
Type Safety ⚠️ Needs Work Type hints removed
Documentation ✅ Good Docstrings updated, could add examples

Recommendations 🎯

  1. Add type hints back (see suggestion above) - This is the main improvement needed
  2. Consider using rl.seconds_per_year in the __main__ example for cleaner code
  3. Optional: Add a docstring example to one function showing mpmath usage with high precision (similar to the PR description example)
  4. Optional: Consider adding a test that verifies the output precision actually exceeds float precision (e.g., check that result has more significant digits than a float could represent)

Conclusion 📝

This is a high-quality PR that successfully achieves its goal of upgrading propulsion.py to use mpmath. The implementation is correct, well-tested, and maintains API compatibility. The only significant issue is the removal of type hints, which should be addressed to maintain code quality and developer experience.

Recommendation: Approve with minor changes (add type hints back)


@lookbusy1344
Copy link
Owner

@copilot let’s use years = t / rl.seconds_per_year in the main section, as suggested

Co-authored-by: lookbusy1344 <3680611+lookbusy1344@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 17, 2025

@copilot let’s use years = t / rl.seconds_per_year in the main section, as suggested

Done. Updated both lines in the main section to use rl.seconds_per_year (commit b8afd97).

@lookbusy1344 lookbusy1344 marked this pull request as ready for review November 17, 2025 22:54
@lookbusy1344 lookbusy1344 merged commit 4a8dc5d into main Nov 17, 2025
5 checks passed
@lookbusy1344 lookbusy1344 deleted the copilot/upgrade-propulsion-to-mpmath branch November 17, 2025 22:55
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.

Propulsion.py to use mpmath

2 participants