Skip to content

Conversation

Copy link

Copilot AI commented Dec 8, 2025

Pull request type

  • Code maintenance (refactoring, formatting, tests)

Checklist

  • Tests for the changes have been added (if needed)
  • Lint (black rocketpy/ tests/) has passed locally
  • All tests (pytest tests -m slow --runslow) have passed locally

Current behavior

No acceptance tests exist for 3DOF flight simulation feature. Integration tests exist but lack end-to-end validation with realistic scenarios.

New behavior

Adds comprehensive acceptance test suite (tests/acceptance/test_3dof_flight.py) with 12 tests covering:

  • Trajectory validation: Apogee, velocity, flight time within expected ranges
  • Energy conservation: Kinetic/potential energy behavior through flight phases
  • Weathercocking physics verification: Tests verify that weathercocking implementation causes trajectory changes (coefficients 0.0, 0.5, 1.0, 2.0)
  • Mass variation: Propellant consumption and post-burnout behavior
  • Reproducibility: Identical inputs yield identical outputs

Tests use realistic Bella Lui rocket parameters for acceptance testing baseline:

  • Motor: AeroTech K828FJ (~888N avg thrust, 2.43s burn time, 1.373kg propellant, 1.0kg dry mass)
  • Rocket: 78mm radius (156mm diameter), 17.227kg dry mass, 0.43 drag coefficients
  • Actual Performance: Bella Lui flight data shows ~459m apogee

Tolerance ranges calibrated to Bella Lui performance:

  • Apogee altitude: 300-600m (matches actual ~459m performance)
  • Apogee time: 5-30s (appropriate for this rocket class)
  • Velocity: 30-150 m/s (subsonic performance)
  • Ranges allow for point mass approximation variations and atmospheric conditions

Named constants throughout: All magic numbers extracted to meaningful constants with clear documentation:

  • Test tolerances and ranges (MIN/MAX_APOGEE_ALTITUDE, MIN/MAX_VELOCITY, etc.)
  • Weathercock coefficients (WEATHERCOCK_COEFFICIENTS)
  • Launch parameters (LAUNCH_INCLINATION=85°, LAUNCH_HEADING=0°) defined at module level
  • Mass and thrust tolerances (MASS_TOLERANCE=0.001kg, THRUST_TOLERANCE=1e-6N)
  • Quaternion tolerance (QUATERNION_CHANGE_TOLERANCE=0.2) accounts for passive aerodynamic effects
  • Zero duplication - shared constants imported from fixtures

Physics-based weathercocking verification: Rather than checking specific tolerance thresholds (which are hard to quantify for weathercocking effects), tests verify that:

  • Weathercocking causes trajectory changes (apogee and lateral displacement differ between weathercock_coeff=0.0 and weathercock_coeff=1.0)
  • Different weathercock coefficients produce measurable variation (range-based checks with 0.01m apogee tolerance and 0.001m lateral tolerance)
  • The physics implementation is working correctly without asserting unrealistic quantitative thresholds

Tolerance-based assertions: All non-weathercocking comparisons use explicit named tolerances with clear error messages showing expected vs actual values with tolerance context.

Critical bug fixes addressed during review:

  • Rocket radius correction: Fixed critical bug where radius was 0.156m (312mm diameter) instead of correct 0.078m (156mm diameter) - this was affecting drag calculations significantly
  • Floating-point comparisons: Replaced set() uniqueness checks with range-based comparisons to avoid floating-point precision issues
  • Quaternion tolerance: Increased tolerance to 0.2 to account for passive aerodynamic effects, numerical integration, and wind effects that occur even without active weathercocking

Fixture Organization: All 3DOF-specific fixtures centralized in tests/fixtures/flight/flight_fixtures.py:

  • acceptance_point_mass_motor: Motor based on K828FJ specifications with Bella Lui parameters
  • acceptance_point_mass_rocket: Rocket based on Bella Lui parameters (corrected radius)
  • flight_3dof_no_weathercock: 3DOF flight without weathercocking (weathercock_coeff=0.0)
  • flight_3dof_with_weathercock: 3DOF flight with weathercocking enabled (weathercock_coeff=1.0)

The fixtures reuse existing example_spaceport_env from environment_fixtures.py. All fixtures properly import and utilize PointMassMotor and PointMassRocket from the codebase.

Breaking change

  • No

Additional information

Tests are fully executable using the existing 3DOF implementation in the codebase (rocketpy/motors/point_mass_motor.py and rocketpy/rocket/point_mass_rocket.py). All existing acceptance tests (bella_lui, ndrt_2020, prometheus) pass unchanged. CodeQL: 0 alerts.

Fixtures follow the project's pattern of centralizing test fixtures in tests/fixtures/ directory. Imports are properly structured at the top of files following Python conventions. All constants are appropriately named for their specific purpose with zero duplication across files.

Tolerance ranges are based on actual Bella Lui flight data analysis, ensuring realistic expectations while allowing for the point mass approximation and atmospheric variations inherent in 3DOF simulations. Weathercocking tests verify physics implementation by checking for trajectory changes rather than asserting specific quantitative thresholds that are difficult to determine realistically. Quaternion tests account for passive aerodynamic effects that occur even without active weathercocking model.

Original prompt

create a new branch: tst/3dof-acceptance-test which implements and solves the following Issue: #887. Also open a new PR with base branch as develop.


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

@aZira371 aZira371 changed the base branch from master to develop December 8, 2025 06:38
Copilot AI changed the title [WIP] Implement 3DOF acceptance test TST: Add acceptance tests for 3DOF flight simulation Dec 8, 2025
Copilot AI requested a review from aZira371 December 8, 2025 06:45
@aZira371
Copy link
Collaborator

aZira371 commented Dec 8, 2025

@copilot Please check if the fixtures from flight_fixtures.py can be reused for these tests or not. If yes then reuse them here. If not then add new fixtures to the flight_fixtures.py file.

Copy link
Author

Copilot AI commented Dec 8, 2025

@copilot Please check if the fixtures from flight_fixtures.py can be reused for these tests or not. If yes then reuse them here. If not then add new fixtures to the flight_fixtures.py file.

Done! I've refactored the fixtures as requested:

Changes made (commit c6cbc90):

  • ✅ Removed acceptance_environment fixture from test file
  • ✅ Now reusing existing example_spaceport_env from environment_fixtures.py
  • ✅ Moved all 3DOF-specific fixtures to flight_fixtures.py:
    • acceptance_point_mass_motor
    • acceptance_point_mass_rocket
    • flight_3dof_no_weathercock
    • flight_3dof_with_weathercock

Results:

  • Test file reduced from 545 to 441 lines
  • All fixtures now centralized in flight_fixtures.py for better reusability
  • Fixtures use conditional import so they only load when 3DOF classes are available

@aZira371
Copy link
Collaborator

@copilot Pointmassrocket and pointmassmotor are avialable in the base branch as well as the branch you have created. They should be utlised here for 3 dof flight accpetance tests. As they form the baseline of flight 3 dof sims. The flight fixtures already have acceptance forms of both the pointmassmotor and pointmassrocket. so why are they being called in the test_3dof_flight.py. Seems redundant. Analyze this line of thought in a critic sense and make changes. Similary why implement try and except for both the classes when they are available within the code base. check the codebase thoroughly please.

Copy link
Author

Copilot AI commented Dec 14, 2025

@copilot Pointmassrocket and pointmassmotor are avialable in the base branch as well as the branch you have created. They should be utlised here for 3 dof flight accpetance tests. As they form the bas...

You're absolutely right! I've analyzed the codebase thoroughly and made the necessary changes (commit 7eab61d):

Issues found and fixed:

  1. ✅ PointMassMotor and PointMassRocket ARE available in the codebase at rocketpy/motors/point_mass_motor.py and rocketpy/rocket/point_mass_rocket.py
  2. ✅ They're properly exported in rocketpy/__init__.py
  3. ✅ The try/except blocks were unnecessary and misleading
  4. ✅ The conditional if THREEDOF_AVAILABLE: wrapper was redundant
  5. ✅ The pytestmark skip decorator was preventing tests from running

Changes made:

  • Removed all try/except import blocks from both files
  • Removed conditional fixture definitions - fixtures are now always available
  • Removed pytestmark skip decorator from test file
  • Moved imports to top of file per Python conventions
  • Tests will now execute properly using the existing 3 DOF implementation

The fixtures properly utilize PointMassMotor and PointMassRocket as the baseline for 3 DOF flight simulations as intended.

Copy link
Collaborator

@aZira371 aZira371 left a comment

Choose a reason for hiding this comment

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

@copilot Please review the comments and implement requested changes, if applicable.

Copy link
Author

Copilot AI commented Dec 14, 2025

@copilot Please review the comments and implement requested changes, if applicable.

All requested changes have been implemented in commit 85a0eff:

  1. ✅ Fixtures based on Bella Lui rocket parameters
  2. ✅ Basic trajectory test updated for Bella Lui
  3. ✅ Magic numbers extracted to named constants
  4. ✅ Tolerance-based assertions throughout

The tests now provide realistic acceptance testing using the Bella Lui rocket as a baseline, with all parameters properly documented and all magic numbers replaced with meaningful constants.

Copilot AI changed the title TST: Add acceptance tests for 3DOF flight simulation TST: Add acceptance tests for 3DOF flight simulation based on Bella Lui rocket Dec 14, 2025
Copilot AI requested a review from aZira371 December 14, 2025 18:59
@aZira371
Copy link
Collaborator

@copilot The tolerances related to weathercocking are not realistic. The changes in flight trajectory due to weathercoking are hard to quantify on, instead of checking for tolerances there, we should ideally track if the properties are changing or not. The tests which check the effect for weathercocking should probably look for evaluating whether the physics implementation is happening or not.

Copy link
Author

Copilot AI commented Dec 14, 2025

@copilot The tolerances related to weathercocking are not realistic. The changes in flight trajectory due to weathercoking are hard to quantify on, instead of checking for tolerances there, we should ...

Agreed! I've refactored the weathercocking tests to verify physics implementation rather than check specific tolerances (commit 0f765af).

Changes:

  • Removed WEATHERCOCK_APOGEE_DIFFERENCE and WEATHERCOCK_RANGE_THRESHOLD constants
  • test_3dof_weathercocking_affects_trajectory: Now checks apogee_no_wc != apogee_with_wc and lateral_no_wc != lateral_with_wc to verify weathercocking causes changes
  • test_3dof_flight_different_weathercock_coefficients: Uses len(unique_apogees) > 1 to verify different coefficients produce different results

Tests now verify that weathercocking physics is being applied (trajectory changes occur) without asserting specific magnitude thresholds that are hard to quantify.

@codecov
Copy link

codecov bot commented Dec 14, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.10%. Comparing base (9cf3dd4) to head (983db0c).
⚠️ Report is 27 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #914      +/-   ##
===========================================
+ Coverage    80.27%   81.10%   +0.83%     
===========================================
  Files          104      107       +3     
  Lines        12769    13833    +1064     
===========================================
+ Hits         10250    11219     +969     
- Misses        2519     2614      +95     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds comprehensive acceptance tests for the 3DOF (3 Degrees of Freedom) flight simulation mode in RocketPy. The tests validate trajectory physics, energy conservation, weathercocking behavior, mass variation, and reproducibility using realistic Bella Lui rocket parameters.

  • Adds 12 new acceptance tests covering key flight physics and edge cases for 3DOF simulations
  • Introduces test fixtures for point mass motors and rockets based on AeroTech K828FJ motor specifications
  • Establishes baseline acceptance criteria with calibrated tolerance ranges based on actual Bella Lui flight data (~459m apogee)

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File Description
tests/fixtures/flight/flight_fixtures.py Adds 6 new fixtures for 3DOF testing: motor and rocket configurations based on Bella Lui parameters, plus flight fixtures with and without weathercocking. Also defines module-level launch parameter constants.
tests/acceptance/test_3dof_flight.py New test suite with 12 acceptance tests validating trajectory, energy conservation, weathercocking physics, mass variation, thrust profiles, quaternion evolution, and reproducibility for 3DOF flight simulations.

Copy link
Member

@Gui-FernandesBR Gui-FernandesBR left a comment

Choose a reason for hiding this comment

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

@aZira371 good work!

Please address all the comments created by copilot so we can proceed with merging this PR

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

- MNT: added required line to changelog.md
Copy link
Collaborator

@aZira371 aZira371 left a comment

Choose a reason for hiding this comment

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

@Gui-FernandesBR Changes made according to copilot review with my suggestions as response. PR should be good to go. Change log has been updated as well. Running one last commit to close everything.

@aZira371 aZira371 merged commit 7a10e29 into develop Dec 23, 2025
10 checks passed
@aZira371 aZira371 deleted the copilot/implement-3dof-acceptance-test branch December 23, 2025 19:53
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.

3 participants