-
-
Notifications
You must be signed in to change notification settings - Fork 214
ENH: Add multi-dimensional drag coefficient support (Cd as function of M, Re, α) #875
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
base: develop
Are you sure you want to change the base?
Conversation
|
The code implementation is as I planned it to be done. There might be a slight edge case which should be addressed in regular grid interpolator unit tests or with warning: that the grid was not sorted in ascending order as intended. This can happen with repeated values in the grid. Apart from that the changes are okay to be merged! @Gui-FernandesBR @copilot |
356e14a to
b621a16
Compare
|
@Gui-FernandesBR Is there a way to see this branch on VSC? I am unable to access it there. It is not being listed as a remote branch. I think one of the previous lint errors was related to the new grid method being too big in function.py. It should be ideally broken down into helper functions or pylint has to be disabled for it. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #875 +/- ##
===========================================
+ Coverage 80.27% 80.36% +0.08%
===========================================
Files 104 106 +2
Lines 12769 13122 +353
===========================================
+ Hits 10250 10545 +295
- Misses 2519 2577 +58 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
The pull request is ready for review now. Pylint is failing due to a problem in .pylintrc, I am not sure if this file should be corrected in this PR or not as this is an unrelated error. Tests did pass locally on my end before merging from develop. @RocketPy-Team/code-owners will need help from you, because different tests are failing at different places - on github test_sensor failed but on local after merging develop test_environment failed. |
Co-authored-by: Gui-FernandesBR <[email protected]>
…tegration tests Co-authored-by: Gui-FernandesBR <[email protected]>
Co-authored-by: Gui-FernandesBR <[email protected]>
- MNT: velocity_body was not being used in get_drag_coefficient, removed it as an input
… test_multidim_drag.py -MNT: removed unused velocity in body frame parameter requirement from all instances of get_drag_coefficient in flight.py - MNT: corrected docstring for get_value_opt_grid in function.py - MNT: shifted import of classes before the definition of functions in test_multidim_drag.py
- MNT: rearranged the docstring of from_grid in function.py to match the expected output of doctest
- MNT: reran make format and lint on function.py to correct after previous changes to from_grid
- MNT: disables pylint unused private member for get_value_opt_grid as it is called upon dynamically by from_grid - MNT: disabled pylint too many statement for from_grid for now and added a to-do to refactor it into smaller methods/helper functions - MNT: updated .pylintrc to record Re as good name
- MNT: Re variable was unused in test_3d_drag_with_varying_alpha thus replaced it
* Add tests for shepard_fallback in test_function_grid.py Co-authored-by: aZira371 <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: aZira371 <[email protected]>
52bfc54 to
74fe825
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aZira371 is linear_grid really the best name here? regular_grid should be more similar to scipy, no?
There was a problem hiding this 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 implements multi-dimensional drag coefficient support, allowing drag to vary with Mach number, Reynolds number, and angle of attack. This enhancement enables RocketPy users to utilize CFD simulation data that naturally depends on multiple flow parameters, significantly improving aerodynamic modeling accuracy.
Key changes:
- Added
Function.from_grid()class method for creating N-dimensional structured grid interpolators usingscipy.interpolate.RegularGridInterpolator - Modified Flight class to automatically calculate Reynolds number and angle of attack during trajectory integration, passing these to multi-dimensional drag functions
- Updated Rocket class to preserve Function objects when provided directly as drag coefficients (backward compatible with existing 1D usage)
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
rocketpy/mathutils/function.py |
Adds from_grid() class method, linear_grid interpolation type, and __get_value_opt_grid() for structured grid evaluation with edge-clamping extrapolation |
rocketpy/simulation/flight.py |
Implements __get_drag_coefficient() helper to detect function dimensionality and calculate Re/α during integration; updates all u_dot methods |
rocketpy/rocket/rocket.py |
Preserves Function objects when passed as drag coefficients instead of wrapping them, enabling multi-dimensional drag |
tests/unit/mathutils/test_function_grid.py |
Comprehensive unit tests for grid interpolation (1D/2D/3D), extrapolation modes, validation, and fallback behavior |
tests/integration/test_multidim_drag.py |
Integration tests verifying 3D drag works in full flight simulations and maintains backward compatibility with 1D drag |
.pylintrc |
Adds 'Re' (Reynolds number) to accepted variable names |
| def test_shepard_fallback_warning(): | ||
| """Test that shepard_fallback is triggered and emits a warning. | ||
| When linear_grid interpolation is set but no grid interpolator is available, | ||
| the Function class should fall back to shepard interpolation and emit a warning. | ||
| """ |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring for test_shepard_fallback_warning states that "the Function class should fall back to shepard interpolation and emit a warning," but the test implementation doesn't actually test the fallback scenario correctly. The test manually changes interpolation to 'linear_grid' on a Function that wasn't created with from_grid(), which is not a realistic scenario.
Consider renaming the test to better reflect what it actually tests (e.g., test_linear_grid_without_grid_interpolator_warns) or restructure it to test a more realistic fallback scenario if one exists.
| mu = self.env.dynamic_viscosity.get_value_opt(z) | ||
| freestream_speed = np.linalg.norm(freestream_velocity_body) | ||
| characteristic_length = 2 * self.rocket.radius # Diameter | ||
| reynolds = rho * freestream_speed * characteristic_length / mu |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential division by zero: reynolds = rho * freestream_speed * characteristic_length / mu on line 1412 doesn't check if mu (dynamic viscosity) is zero or near-zero before division. While atmospheric models typically provide non-zero viscosity values, defensive programming suggests adding a check or using a safe division operation to prevent crashes in edge cases.
Consider: reynolds = rho * freestream_speed * characteristic_length / max(mu, 1e-10) or adding an explicit check.
| reynolds = rho * freestream_speed * characteristic_length / mu | |
| # Defensive: avoid division by zero in case mu is zero or near-zero | |
| reynolds = rho * freestream_speed * characteristic_length / max(mu, 1e-10) |
| def test_3d_drag_with_varying_alpha(): | ||
| """Test that 3D drag responds to angle of attack changes.""" | ||
| # Create drag function with strong alpha dependency | ||
| mach = np.array([0.0, 0.5, 1.0, 1.5]) | ||
| reynolds = np.array([1e5, 1e6]) | ||
| alpha = np.array([0.0, 5.0, 10.0, 15.0]) | ||
|
|
||
| M, _, A = np.meshgrid(mach, reynolds, alpha, indexing="ij") | ||
| # Strong alpha dependency: Cd increases significantly with alpha | ||
| cd_data = 0.3 + 0.05 * M + 0.03 * A | ||
| cd_data = np.clip(cd_data, 0.2, 2.0) | ||
|
|
||
| drag_func = Function.from_grid( | ||
| cd_data, | ||
| [mach, reynolds, alpha], | ||
| inputs=["Mach", "Reynolds", "Alpha"], | ||
| outputs="Cd", | ||
| ) | ||
|
|
||
| # Test at different angles of attack | ||
| # At zero alpha, Cd should be lower | ||
| cd_0 = drag_func(0.8, 5e5, 0.0) | ||
| cd_10 = drag_func(0.8, 5e5, 10.0) | ||
|
|
||
| # Cd should increase with alpha | ||
| assert cd_10 > cd_0 | ||
| assert cd_10 - cd_0 > 0.2 # Should show significant difference |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test test_3d_drag_with_varying_alpha verifies that the drag function responds to different alpha values when called directly, but doesn't actually verify that the flight simulation is using angle of attack correctly during integration. Consider adding assertions that check the drag coefficient values during an actual flight simulation (e.g., by examining flight data or verifying that a rocket with high-alpha-sensitivity drag behaves differently than one without).
| extrapolation : str, optional | ||
| Extrapolation behavior. Default is 'constant', which clamps to edge values. | ||
| 'constant': Use nearest edge value for out-of-bounds points. | ||
| 'zero': Return zero for out-of-bounds points. |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring states that extrapolation can be 'constant' or 'zero', but the __get_value_opt_grid method (line 726-727) allows other extrapolation types (like 'natural') to fall through to the interpolator's default behavior. The docstring should document what happens with 'natural' extrapolation (linear extrapolation from edge gradients) or explicitly state that only 'constant' and 'zero' are supported for grid interpolation.
Additionally, consider adding validation in from_grid() to warn or error if an unsupported extrapolation type is specified.
| func.y_array = func._image[: len(axes[0])] # Placeholder | ||
| func.y_initial, func.y_final = func._image[0], func._image[-1] | ||
| if len(axes) > 2: |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential issue with y_array initialization for multi-dimensional grids. Line 4207 sets func.y_array = func._image[: len(axes[0])] which only takes the first n values from the flattened grid where n is the length of the first axis. This creates an inconsistent y_array that doesn't properly represent the function data.
For grid functions with dimensions > 1, y_array is not a meaningful concept (it's designed for 1D functions). Consider either:
- Setting
y_arrayto the full flattened image:func.y_array = func._image - Setting it to None or an empty array with a comment explaining that y_array is not applicable for grid functions
- Documenting this limitation clearly
This could cause issues if other parts of the code expect y_array to contain all function values.
| func.y_array = func._image[: len(axes[0])] # Placeholder | |
| func.y_initial, func.y_final = func._image[0], func._image[-1] | |
| if len(axes) > 2: | |
| # For 1D functions, y_array contains the function values. |
| mesh = np.meshgrid(*axes, indexing="ij") | ||
| domain_points = np.column_stack([m.ravel() for m in mesh]) | ||
| func._domain = domain_points | ||
| func._image = grid_data.ravel() | ||
|
|
||
| # Set source as flattened data array (for compatibility with serialization, etc.) | ||
| func.source = np.column_stack([domain_points, func._image]) |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performance consideration: Creating the full flattened domain and image arrays (lines 4185-4191) for compatibility purposes requires O(n^d) memory where n is the typical axis length and d is the number of dimensions. For large 3D grids (e.g., 100x100x100 points), this creates redundant data since the grid interpolator already has the structured data.
While this is necessary for backward compatibility with code expecting _domain and _image attributes, consider documenting this memory overhead and potentially adding a parameter to skip this flattening if compatibility with existing Function methods is not required.
| # Determine which parameters to pass based on input names | ||
| input_names = [name.lower() for name in drag_function.__inputs__] | ||
| args = [] | ||
|
|
||
| for name in input_names: | ||
| if "mach" in name or name == "m": | ||
| args.append(mach) | ||
| elif "reynolds" in name or name == "re": | ||
| args.append(reynolds) | ||
| elif "alpha" in name or name == "a" or "attack" in name: | ||
| args.append(alpha_deg) | ||
| else: | ||
| # Unknown parameter, default to mach | ||
| args.append(mach) |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The input parameter name matching uses lowercase comparison and substring matching (e.g., "mach" in name, "reynolds" in name). While functional, this could match unintended parameter names (e.g., "machining" would match "mach").
Consider using more precise matching such as:
- Exact matches:
name in ['mach', 'm'] - Or document the substring matching behavior clearly in the method docstring
This is a minor robustness improvement for edge cases.
| Drag coefficient value | ||
| """ | ||
| # Check if drag function is multi-dimensional by examining its inputs | ||
| if hasattr(drag_function, "__inputs__") and len(drag_function.__inputs__) > 1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not confortable with this comparision here.
if hasattr(drag_function, "__inputs__") and len(drag_function.__inputs__) > 1:This could be done within the Function class.
An attribute called is_multidimensioanl: bool should be really useful.
Then the comparision here would be:
if isinstance(foo, Function) and foo.is_multidimensional:
muche easier to read!
Gui-FernandesBR
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- @MateusStano please review changes in the flight.py file
- @phmbressan please review changes in the function.py ile
Everything is looking good, @aZira371 great work here! I'm confortable with what has been done so far. I believe we can include this one on the next release.
@aZira371 could you please update changelog?
|
Thanks for the review @Gui-FernandesBR will soon work on this and update the changelog. |
Pull request type
Checklist
black rocketpy/ tests/) has passed locallypytest tests -m slow --runslow) have passed locallyCHANGELOG.mdhas been updated (if relevant)Current behavior
Drag coefficients are limited to 1D functions of Mach number. CFD simulations typically produce drag data varying with Reynolds number and angle of attack, which cannot be properly represented.
New behavior
Drag coefficients now support N-dimensional structured grids via
Function.from_grid(). Flight simulations automatically calculate and pass Reynolds number (from velocity, density, viscosity, rocket diameter) and angle of attack (from freestream velocity vectors in body frame) to multi-dimensional drag functions.Implementation
Function Class (
rocketpy/mathutils/function.py)from_grid()class method usingscipy.interpolate.RegularGridInterpolator'linear_grid'interpolation type with edge clamping (no extrapolation beyond bounds)Flight Class (
rocketpy/simulation/flight.py)__get_drag_coefficient()helper detecting function dimensionalityudot_rail1(),u_dot(),u_dot_generalized()to use helperRocket Class (
rocketpy/rocket/rocket.py)Usage Example
Testing
Function.from_grid()covering 1D/2D/3D grids, extrapolation, validationBreaking change
Existing 1D drag curves (Mach only) work unchanged. Multi-dimensional support is opt-in via
from_grid().Additional information
Resolves #874
CodeQL security scan: 0 alerts
Original prompt
This section details on the original issue you should resolve
<issue_title>ENH: Drag curve as a function of Re and alpha as well</issue_title>
<issue_description>Is your feature request related to a problem? Please describe.
As of now drag curve is a function of speed (or mach number). Since a lot of rocketeers use CFD simulations, it is expected that drag values won't be just a function of mach number. Drag of any body moving in fluid can depend on many major parameters. Some of them are listed below:
Re accounts for the viscous effects of the flow (skin friction vs. pressure drag). Reynolds number depends on velocity, characteristic length, air viscosity, and density, and is crucial for modeling drag at low speeds and during laminar-turbulent transitions. Non-zero angles of attack change the effective cross-sectional area and cause flow separation or vortex formation, increasing drag. Altitude or atmospheric variation can be handled by RocketPy separately to update Re or atmospheric data rather than being built directly into the drag curve.
Drag could be input as Cd(M, α) or Cd(M, Re, α).
Describe the solution you'd like
Additional context
It does not make sense that the drag curve will be given as an input of Cd=f(Re) or Cd=f(α). Physically these are not capturing the entire flow physics at the same accuracy at which Cd=f(M) does as mach number based effects like shock wave propagation become more important in rocket flights. May be still allow for single array functionality with Re and α but provide a note that the results may not be accurate!</issue_description>
<agent_instructions>Here is a specific, technical prompt you can use to delegate this issue to the GitHub Copilot agent. This prompt directs it to the correct implementation strategy we discussed (enhancing the
Functionclass).Optional Prompt for GitHub Copilot
Here is a specific prompt to guide the implementation for Issue #874.
1. Core Objective:$\alpha$ ).
Implement the enhancement described in issue #874. The goal is to allow the drag coefficient
Cdto be defined as a 3D function of Mach (M), Reynolds number (Re), and Angle of Attack (2. Implementation Location:
The primary changes must be made within the
rocketpy.Functionclass (inrocketpy/function.py). This is not just a change to theFlightorRocketclass; theFunctionclass itself must be enhanced to handle N-dimensional structured grid data.3. Add New Interpolation Method:
The current
shepardinterpolation is inefficient for structured CFD grids.interpolation='linear_grid'orinterpolation='grid') to theFunctionclass__init__.scipy.interpolate.RegularGridInterpolatoras its backend. You will need to import this fromscipy.interpolate.Functionclass state should store the initializedRegularGridInterpolatorobject to be called later.4. Add New
Function.from_grid()Class Method:Manually creating a flattened list of points is impractical for users.
@classmethod def from_grid(cls, grid_data, axes, inputs, output, interpolation='linear_grid', **kwargs).grid_data: An N-dimensional NumPy array (e.g., a 3D array foraxes: A list of 1D arrays representing the axes of the grid (e.g.,[M_axis, Re_axis, A_axis]).inputs: A list of strings, e.g.,['Mach', 'Reynolds', 'Alpha'].output: A string, e.g.,'Cd'.scipy.interpolate.RegularGridInterpolatorwith the providedaxesandgrid_dataand store it in the newFunctionobject.5. Handle Extrapolation (Crucial):
The new grid interpolation must clamp to the edge (i.e., use the nearest edge value) for out-of-bounds requests. It should not extrapolate.
RegularGridInterpolator, setbounds_error=False.fill_valueshould be set toNone, which (whenbounds_error=False) causes the interpolator to use the edge values for out-of-bounds points.6. Update Flight Class:
The
Flightclass (likely inflight.pyor related files) must be modified to use this new function.💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.