draft: feat/precession - #54
Draft
GeorgGrassler wants to merge 53 commits into
Draft
Conversation
GeorgGrassler
marked this pull request as draft
April 6, 2026 11:37
Review Summary by QodoAdd precession module with magnetic well and turning point analysis
WalkthroughsDescription• Implement precession module with magnetic well analysis - Core functions for finding magnetic well bottom and turning points - Integration grid setup for bounce and pitch angle calculations • Refactor find_global_extrema to find_global_minimum - Changed to return location of minimum instead of min/max values - Updated tolerance parameter from abstol to reltol • Add comprehensive integration tests for precession calculations - Test turning points with mock field - Test precession correction with analytic field Diagramflowchart LR
A["Precession Module"] --> B["Find Magnetic Well Bottom"]
A --> C["Find Turning Points"]
A --> D["Set Integration Grids"]
B --> E["find_global_minimum"]
C --> E
F["Integration Tests"] --> A
File Changes1. src/precession/precession.f90
|
Code Review by Qodo
1. compute_precession_correction misindented declaration
|
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
|
||
| contains | ||
|
|
||
| subroutine compute_precession_correction(field, fieldlines, l_c, Omega_hat, correction) |
There was a problem hiding this comment.
1. compute_precession_correction misindented declaration 📘 Rule violation ⚙ Maintainability
The new compute_precession_correction subroutine declaration is indented with 2 spaces instead of the required 4-space indentation. This breaks formatting consistency and increases diff noise across the Fortran codebase.
Agent Prompt
## Issue description
A new Fortran line is indented with 2 spaces instead of the required 4-space indentation.
## Issue Context
Repository style (and CLAUDE.md) specifies 4-space indentation for Fortran sources.
## Fix Focus Areas
- src/precession/precession.f90[29-29]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+210
to
+215
| extremum_locations = find_global_minimum(lambda_squared_along_fieldline, interval, reltol) | ||
| phi_turning(1) = extremum_locations(1) + (interval(2) - interval(1))*reltol | ||
|
|
||
| interval(1) = fieldline%phi_min | ||
| interval(2) = fieldline%phi_max(2) | ||
| extremum_locations = find_global_minimum(lambda_squared_along_fieldline, interval, reltol) |
There was a problem hiding this comment.
2. Long lines in find_turning_points 📘 Rule violation ⚙ Maintainability
Two newly added lines exceed the 88-character maximum line length, reducing readability and violating the formatting standard. These should be wrapped using Fortran continuation (&) and aligned consistently.
Agent Prompt
## Issue description
New Fortran source lines exceed the 88-character limit.
## Issue Context
The project code style requires max 88 characters per line; long procedure calls should be wrapped with `&` continuation and aligned.
## Fix Focus Areas
- src/precession/precession.f90[210-216]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+27
to
+28
| shaing_callen_lib | ||
| precession_lib) |
There was a problem hiding this comment.
3. Docs not updated for precession 📘 Rule violation ⚙ Maintainability
The PR adds a new precession_lib library/module and new integration tests, but repository documentation (CLAUDE.md and relevant READMEs) was not updated to reflect these architecture/test changes. This can cause developer guidance and documented architecture to drift from the actual build/test graph.
Agent Prompt
## Issue description
Documentation was not updated alongside new architecture and test additions (`precession_lib`, new integration tests).
## Issue Context
CLAUDE.md explicitly requires updates when adding/removing/renaming modules/libraries and when adding tests/labels.
## Fix Focus Areas
- CLAUDE.md[29-56]
- CLAUDE.md[76-85]
- CLAUDE.md[105-109]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+130
to
+151
| function find_global_minimum(func, interval, reltol) result(min_loc) | ||
| use utils, only: linspace | ||
|
|
||
| procedure(func1d) :: func | ||
| real(dp), intent(in) :: interval(2) | ||
| real(dp), intent(in), optional :: abstol | ||
| real(dp) :: extrema(2) | ||
| real(dp), intent(in), optional :: reltol | ||
| real(dp) :: min_loc | ||
|
|
||
| integer :: n_steps | ||
| real(dp), dimension(:), allocatable :: x, value | ||
|
|
||
| if (present(abstol)) then | ||
| n_steps = int(abs(interval(2) - interval(1))/abstol) + 2 | ||
| if (present(reltol)) then | ||
| n_steps = int(1.0_dp/reltol) + 2 | ||
| else | ||
| n_steps = 1000 | ||
| end if | ||
|
|
||
| allocate (x(n_steps), value(n_steps)) | ||
| call linspace(interval(1), interval(2), n_steps, x) | ||
| call func(x, value) | ||
| extrema(1) = minval(value) | ||
| extrema(2) = maxval(value) | ||
| end function find_global_extrema | ||
| min_loc = x(minloc(value, dim=1)) | ||
| end function find_global_minimum |
There was a problem hiding this comment.
4. Removed api breaks build 🐞 Bug ≡ Correctness
find_global_extrema is still imported/called by fieldline_labels.f90, but the find_extrema module no longer defines it, so fieldline_lib will fail to compile.
Agent Prompt
### Issue description
`src/fieldline/fieldline_labels.f90` depends on `find_extrema::find_global_extrema`, but that procedure was removed/renamed in `src/utils/find_extrema.f90`, causing compilation failure.
### Issue Context
- Existing code uses `find_global_extrema` to return `[minval, maxval]` of a sampled function over an interval.
- New code introduces `find_global_minimum` returning the *location* of the minimum, which is not a drop-in replacement.
### Fix Focus Areas
- Re-introduce `find_global_extrema` (old behavior) alongside `find_global_minimum`, or update all call sites and provide an equivalent API for extrema values.
- file/path references:
- src/utils/find_extrema.f90[130-151]
- src/fieldline/fieldline_labels.f90[85-147]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+56
to
+64
| function get_fieldline_at_global_maximum(fieldlines) result(max_fieldline) | ||
| class(fieldline_t), dimension(:), intent(in) :: fieldlines | ||
| type(fieldline_t) :: max_fieldline | ||
|
|
||
| integer :: max_index | ||
|
|
||
| max_index = maxloc(fieldlines%B_max(1), dim=1) | ||
| max_fieldline = fieldlines(max_index) | ||
| end function get_fieldline_at_global_maximum |
There was a problem hiding this comment.
5. Wrong max fieldline chosen 🐞 Bug ≡ Correctness
get_fieldline_at_global_maximum only considers B_max(1) and ignores B_max(2), so it can select the wrong fieldline when the true global maximum is stored in the second entry.
Agent Prompt
### Issue description
The fieldline selection for the global maximum only scans `B_max(1)`, ignoring `B_max(2)`, so it may pick the wrong fieldline and propagate incorrect downstream calculations.
### Issue Context
`fieldline_t%B_max` is a length-2 array; callers likely expect the maximum over both endpoints/branches.
### Fix Focus Areas
- Compute a per-fieldline maximum over both `B_max` entries, then apply `maxloc` on that value.
- file/path references:
- src/precession/precession.f90[56-64]
- src/fieldline/fieldline.f90[6-14]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
A small lamor radius lets the drifts be small as well. If the time steps given by taumin are too big, the Lagrange interpolation can not properly resolve those small differences. Therefore, one can on one hand increase the lamor radius until the drifts become distinct from the parallel motion and then divide the lamor radius out to get species indpendant values. However, if the drifts are too small, they also effect other quantites i.e. allow trapped particles to escape their wells and become more/less "passing". When wanting to keep the drifts small but still resolve them, one needs to do more steps around the tips to to properly interpolate correctly. Increasing the tolerance of the odeint solvers does not change the result here, as it it not their accurcacy that is the issue, but the missing points to properly resolve the tip (i.e. misssing information).
Resolve conflicts and adapt the precession code to main's APIs: - keep field_3D_t alongside rel_accuracy_B/get_covariant_components in field_base - keep find_global_minimum/find_global_maximum alongside main's find_global_extrema - drop the local boozer_converter.F90 (moved to libneo); keep the use_B_r_covariant init switch - link precession_lib against granular libneo targets instead of LIBNEO::neo - pass flock_of_fieldlines_t through precession, electric_rabe and the precession tests - carry M_pol/N_tor/nfp on fieldline_for_precession_t (moved off fieldline_t to the flock) - implement rel_accuracy_B/get_covariant_components on mock_field_3d_t
gfortran treats line truncation as an error, which failed the CI build.
Member
|
Merged main and resolved the conflicts (field_base, find_extrema, boozer_field, fieldline_labels, surface_average, CMake; dropped the local boozer_converter.F90 in favor of libneo); adapted the precession code and tests to main's flock/libneo APIs and fixed the CI build failure (132-column line truncation in test_trace_till_bounce.f90). 34/35 quick tests pass. TestPrecessionAnalytic still fails and needs a decision on two points:
|
Collaborator
Author
|
@krystophny do not touch this branch please. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.