Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Perhaps wait for the comments from Copilot and add a description. |
There was a problem hiding this comment.
Pull request overview
Fixes incorrect handling of omega / va defaults in the linearize workflow (previously resetting omega to zero in some index configurations) and updates related set_va! semantics and tests to cover non-zero operating points.
Changes:
- Update
linearizeto preserve the initialomega(and use_va) when corresponding index sets arenothing. - Adjust
set_va!(body_aero, va, omega)to always keepbody_aero.vadefined by settinghas_distributed_va=false. - Update/expand tests to use non-zero baseline
(va, theta, delta, omega)and tweak combined-input linearization tolerances.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/solver.jl |
Fixes linearize input application so omitted omega_idxs doesn’t implicitly zero out rotation, and avoids body_aero.va access when it may be undefined. |
src/body_aerodynamics.jl |
Changes set_va!(va, omega) to treat rotation-induced panel variation as non-“distributed inflow” for purposes of .va access. |
test/body_aerodynamics/test_results.jl |
Strengthens coverage with non-zero operating point inputs and updates combined-perturbation acceptance thresholds. |
test/body_aerodynamics/test_body_aerodynamics.jl |
Updates expectations around has_distributed_va / body_aero.va behavior when omega is non-zero. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| va = isnothing(va_idxs) ? init_va : y[va_idxs] | ||
| om = isnothing(omega_idxs) ? init_omega : |
There was a problem hiding this comment.
In calc_results!, y[va_idxs] / y[omega_idxs] create new arrays each call. Since linearize evaluates this many times during finite-difference Jacobian computation, this can add significant allocation overhead. Consider using @views for these slices (and/or view(y, va_idxs) / view(y, omega_idxs)) so set_va! receives a view instead of an allocated copy.
| va = isnothing(va_idxs) ? init_va : y[va_idxs] | |
| om = isnothing(omega_idxs) ? init_omega : | |
| @views va = isnothing(va_idxs) ? init_va : y[va_idxs] | |
| @views om = isnothing(omega_idxs) ? init_omega : |
| body_aero._va .= va | ||
| body_aero.has_distributed_va = false |
There was a problem hiding this comment.
has_distributed_va is now always set to false in the (va, omega) overload, even when omega induces per-panel velocity variation. Since getproperty(::BodyAerodynamics, :va) uses this flag to decide whether .va is well-defined, it would help to clarify (in the set_va! docstring and/or the :va error message) that has_distributed_va only refers to externally supplied per-panel inflow (matrix input), not rotation-induced variation.
| @test lin_prediction ≈ nonlin_res rtol=0.05 atol=1e-3 | ||
| @test error_ratio < 0.05 |
There was a problem hiding this comment.
The combined-input linearization assertions were loosened substantially (rtol 0.01→0.05 and error_ratio 0.005→0.05). This reduces the test’s ability to catch regressions in linearize. If the failures were due to leaving the linear regime, consider keeping a tighter tolerance by reducing combo_scale / perturbation magnitudes, or assert primarily on error_ratio with a smaller bound for at least one representative combined case.
No description provided.