Add ThrustCalculator#1229
Conversation
efaulhaber
force-pushed
the
ef/thrust-calculator
branch
from
189c13b to
5aa80fe
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1229 +/- ##
===========================================
- Coverage 90.05% 70.86% -19.19%
===========================================
Files 136 136
Lines 10594 10596 +2
===========================================
- Hits 9540 7509 -2031
- Misses 1054 3087 +2033
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
efaulhaber
force-pushed
the
ef/thrust-calculator
branch
from
5aa80fe to
fc31569
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
efaulhaber
left a comment
efaulhaber
left a comment
There was a problem hiding this comment.
Codex Review
This is an AI-generated code review. Please verify the findings and summary before acting on them.
Summary
- Adds
MechanicalWorkCalculatoras a general custom quantity forPostprocessCallbackand removes the old callback include. - Adds
ThrustCalculatorandcalculated_thrustfor projected hydrodynamic force calculations. - Updates exports, documentation, and tests/examples to use the new calculator-based postprocessing flow.
| end | ||
|
|
||
| # This should dispatch on `TotalLagrangianSPHSystem`, but this name is not yet defined | ||
| # due to the include order. |
There was a problem hiding this comment.
WARNING: direction_ = SVector(Tuple(direction)) preserves the input vector element type instead of converting it to the system element type. For Float32 systems with default Float64 literals, this leaves calculator.direction as Float64, contradicting the updated test expectation and causing type instability. Convert the direction to ELTYPE before normalization, e.g. build an SVector{ndims(system), ELTYPE}.
efaulhaber
left a comment
efaulhaber
left a comment
There was a problem hiding this comment.
Codex Review
This is an AI-generated code review. Please verify the findings and summary before acting on them.
Summary
- Adds
MechanicalWorkCalculatoras a custom quantity forPostprocessCallbackand removes the old callback include. - Adds
ThrustCalculatorandcalculated_thrustto compute projected hydrodynamic force. - Exports the new calculator APIs and documents mechanical work calculator autodocs.
- Updates mechanical work and example tests to use
PostprocessCallback.
| Get the latest projected hydrodynamic force from a [`ThrustCalculator`](@ref). | ||
| """ | ||
| function calculated_thrust(calculator::ThrustCalculator) | ||
| return calculator.thrust |
There was a problem hiding this comment.
INFO: calculated_thrust is exported public API, but its docstring lacks the required # Arguments section. Add an arguments section documenting calculator, matching the package docstring convention for public functions.
There was a problem hiding this comment.
Pull request overview
This PR replaces the legacy MechanicalWorkCalculatorCallback with a PostprocessCallback-compatible custom quantity (MechanicalWorkCalculator) and introduces a new ThrustCalculator custom quantity for projected hydrodynamic force on structures.
Changes:
- Added
MechanicalWorkCalculatorandThrustCalculatorfunctors (custom quantities) and exported the new APIs. - Removed
MechanicalWorkCalculatorCallbackcallback implementation and updated tests/examples to usePostprocessCallback. - Updated documentation to include the new calculators via autodocs.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/examples/examples.jl | Migrates example smoke tests from MechanicalWorkCalculatorCallback to MechanicalWorkCalculator + PostprocessCallback, and adds a thrust sanity check. |
| test/callbacks/mechanical_work_calculator.jl | Updates unit tests for the new MechanicalWorkCalculator API and adds tests for ThrustCalculator. |
| src/TrixiParticles.jl | Updates exports to expose MechanicalWorkCalculator/ThrustCalculator and their accessors. |
| src/general/mechanical_work_calculator.jl | Adds implementations of MechanicalWorkCalculator and ThrustCalculator plus supporting helpers. |
| src/general/general.jl | Includes the new general/mechanical_work_calculator.jl module file. |
| src/callbacks/mechanical_work_calculator.jl | Removes the old callback-based mechanical work calculator implementation. |
| src/callbacks/callbacks.jl | Drops the include of the removed callback file. |
| docs/src/callbacks.md | Adds autodocs section for the new calculator implementations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Check vector length before converting to `SVector` to avoid extremely long | ||
| # compile times when accidentally passing large vectors. | ||
| if length(direction) != ndims(system) | ||
| throw(ArgumentError("length of `direction` must match the number of dimensions")) | ||
| end | ||
| direction_ = SVector(Tuple(direction)) | ||
| if iszero(direction_) | ||
| throw(ArgumentError("`direction` must not be zero")) | ||
| end | ||
| direction_ = normalize(direction_) |
| # Mechanical Work Calculator | ||
|
|
||
| The `MechanicalWorkCalculator` is a special custom quantity to be used with the | ||
| [`PostprocessCallback`](@ref). | ||
|
|
efaulhaber
left a comment
efaulhaber
left a comment
There was a problem hiding this comment.
Codex Review
This is an AI-generated code review. Please verify the findings and summary before acting on them.
The PR replaces the previous mechanical work callback with a postprocess custom quantity and adds thrust calculation, but it removes the exported callback API without a compatibility path. Existing user code relying on the public callback constructor will break.
|
|
||
| Functor that accumulates the work done by a set of particles in a | ||
| [`TotalLagrangianSPHSystem`](@ref) by integrating the instantaneous power over time. | ||
| It can be passed as a custom quantity to [`PostprocessCallback`](@ref), which controls |
There was a problem hiding this comment.
WARNING: MechanicalWorkCalculator integrates work only when PostprocessCallback invokes it, so the callback interval is also the Euler integration step rather than only an output sampling interval. Document this accuracy tradeoff and point users to write_file_interval when they only want less frequent file output.
| PostprocessCallback, StepsizeCallback, UpdateCallback, SteadyStateReachedCallback, | ||
| SplitIntegrationCallback, MechanicalWorkCalculatorCallback, | ||
| calculated_mechanical_work, | ||
| SplitIntegrationCallback, MechanicalWorkCalculator, calculated_mechanical_work, |
There was a problem hiding this comment.
WARNING: MechanicalWorkCalculator and ThrustCalculator are new exported user-facing quantities, but the PR does not add compact and rich Base.show methods required for types shown in callback summaries. Add display methods using summary_header, summary_line, and summary_footer.
| Pages = ["general/custom_quantities.jl"] | ||
| ``` | ||
|
|
||
| # Mechanical Work Calculator |
There was a problem hiding this comment.
INFO: This autodocs block now includes ThrustCalculator, but the section title and intro only mention mechanical work. Rename or expand the section so users can find thrust documentation under an accurate heading.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 669482115e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| true, | ||
| dv, v_ode, u_ode, semi, 0.0, | ||
| dt3) | ||
| work1 = TrixiParticles.update_mechanical_work!(work1, system, eachparticle, |
There was a problem hiding this comment.
This test now calls TrixiParticles.update_mechanical_work!, but the helper introduced in this commit is named update_mechanical_work (without !). Since test/callbacks/callbacks.jl includes this file, the callbacks test suite will throw UndefVarError as soon as it reaches this testset instead of validating the calculator; use the actual function name or restore a compatibility alias.
Useful? React with 👍 / 👎.
| if length(direction) != ndims(system) | ||
| throw(ArgumentError("length of `direction` must match the number of dimensions")) | ||
| end | ||
| direction_ = SVector(Tuple(direction)) |
There was a problem hiding this comment.
When a Float32 system is constructed with a usual Float64 tuple direction, this line preserves a Float64 direction instead of the calculator/system element type; the added constructor test for system32 therefore fails, and later thrust projections promote to Float64 before being narrowed back into thrust::Float32. Convert the direction entries to ELTYPE before building or normalizing the SVector.
Useful? React with 👍 / 👎.
| if !calculator.initialized | ||
| calculator.t = t | ||
| calculator.work = zero(calculator.work) | ||
| calculator.initialized = true | ||
| return calculator.work |
There was a problem hiding this comment.
If the same MechanicalWorkCalculator object is reused for another solve or a restarted run, initialized remains true from the previous run, so the first postprocess call at the new initial time computes dt = t - calculator.t using the old final time and carries the old accumulated work forward. The previous callback reset t and work during callback initialization; this functor needs an equivalent reset path when PostprocessCallback starts a solve.
Useful? React with 👍 / 👎.
Based on #1228.