Add super-spheroid / super-ellipsoid shapes (Bi & Lin dust models)#13
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThree new superquadric shape types are added to the library: ChangesSuperquadric Shape Implementations
Sequence Diagram(s)No sequence diagrams generated. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13 +/- ##
==========================================
+ Coverage 95.78% 95.89% +0.11%
==========================================
Files 35 38 +3
Lines 4726 4951 +225
==========================================
+ Hits 4527 4748 +221
- Misses 199 203 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/shapes/superellipsoid.jl`:
- Around line 130-135: The current rmin(SuperEllipsoid) returns min(a,b,c) which
is not a conservative inner radius for e,n>1; change rmin to compute a
conservative lower bound using the tighter formula mentioned in the comment:
return min(s.a, s.b, s.c) * 2.0^(- (max(s.e, s.n) - 1) / 2), and ensure you
handle edge cases (e or n <= 1) by falling back to min(a,b,c) or clamping the
exponent so the result remains safe; update the function rmin and reference
fields a, b, c, e, n of SuperEllipsoid accordingly.
In `@src/shapes/superspheroid.jl`:
- Around line 158-165: rmin currently only considers the equatorial diagonal and
polar radius; add the true 3D diagonal check by computing the radius along the
(1,1,1) direction and taking the minimum of a_diag, s.c, and that diagonal. In
function rmin(s::SuperSpheroid) (variables n, a_diag, s.c), compute u =
(1,1,1)/√3 and r_diag = ( (abs(u[1])/s.a)^n + (abs(u[2])/s.a)^n +
(abs(u[3])/s.c)^n )^(-1/n ) (use Float64(s.n) for n), then return min(a_diag,
s.c, r_diag).
In `@src/shapes/superspheroidrevolved.jl`:
- Around line 208-222: The current _superspheroidrevolved_rmax_rmin samples r(ϑ)
at 1001 fixed angles and can miss interior extrema; replace the crude sampling
with a robust 1D optimization of the scalar function r(ϑ) = (sin(ϑ)^p/ap +
cos(ϑ)^p/cp)^(-s.n/2) on ϑ ∈ [0, π/2] (where p = 2/s.n, ap = s.a^p, cp = s.c^p)
to find true global maximum and minimum (e.g. use a reliable bracketing/Brent
method or multiple-start golden-section searches) and fall back to endpoint
values only if the optimizer fails; update _superspheroidrevolved_rmax_rmin to
call that optimizer instead of returning maxima/minima of the fixed rvals array.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: eb607ce1-d49a-4f78-9977-30ca4ff5f21d
📒 Files selected for processing (5)
src/TransitionMatrices.jlsrc/shapes/index.jlsrc/shapes/superellipsoid.jlsrc/shapes/superspheroid.jlsrc/shapes/superspheroidrevolved.jl
Comment on lines
+208
to
+222
| function _superspheroidrevolved_rmax_rmin(s::SuperSpheroidRevolved) | ||
| p = 2 / s.n | ||
| ap = s.a^p | ||
| cp = s.c^p | ||
| # Sample r over ϑ ∈ [0, π/2] (symmetric about equator and pole) | ||
| N = 1000 | ||
| rvals = [begin | ||
| ϑ = i * π / 2 / N | ||
| sinϑ = sin(ϑ) | ||
| cosϑ = cos(ϑ) | ||
| g = sinϑ^p / ap + cosϑ^p / cp | ||
| g^(-s.n / 2) | ||
| end | ||
| for i in 0:N] | ||
| return maximum(rvals), minimum(rvals) |
There was a problem hiding this comment.
Sampled extrema do not satisfy the rmax/rmin contract.
This helper only evaluates 1001 angles, so it can return rmax too small or rmin too large by missing an interior extremum. That is exactly the regime this code is trying to handle for concave shapes, so the discretization defeats the special-case logic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/shapes/superspheroidrevolved.jl` around lines 208 - 222, The current
_superspheroidrevolved_rmax_rmin samples r(ϑ) at 1001 fixed angles and can miss
interior extrema; replace the crude sampling with a robust 1D optimization of
the scalar function r(ϑ) = (sin(ϑ)^p/ap + cos(ϑ)^p/cp)^(-s.n/2) on ϑ ∈ [0, π/2]
(where p = 2/s.n, ap = s.a^p, cp = s.c^p) to find true global maximum and
minimum (e.g. use a reliable bracketing/Brent method or multiple-start
golden-section searches) and fall back to endpoint values only if the optimizer
fails; update _superspheroidrevolved_rmax_rmin to call that optimizer instead of
returning maxima/minima of the fixed rvals array.
Three new shapes for the Bi Lei group's superquadric dust-aerosol family, with the
parameterization verified against Lin & Bi 2018 (JGR, 10.1029/2018JD029464) and
Bi et al. 2018 (Opt. Express, 10.1364/OE.26.001726):
- `SuperSpheroid <: AbstractNFoldShape{4}` — |x/a|^(2/n)+|y/a|^(2/n)+|z/c|^(2/n)=1,
super-circular cross-sections (D4h, 4-fold; NOT axisymmetric), solved via the n-fold
IITM. n=1 spheroid, n=2 octahedron, n>2 concave (dust best-fit n∈[2.4,3.0]).
- `SuperEllipsoid <: AbstractNFoldShape{2}` — full triaxial super-ellipsoid with the two
roundness exponents (e east-west, n north-south), D2h, via the n-fold IITM.
- `SuperSpheroidRevolved <: AbstractAxisymmetricShape` — the true surface of revolution
(ϱ/a)^(2/n)+(z/c)^(2/n)=1, so it flows into EBCM / Sh-matrix / near-field; n=1 reduces
exactly to `Spheroid`.
Each implements the shape interface (∈, refractive_index, volume/volume_equivalent_radius
via the superquadric beta-function formula, rmax, rmin, has_symmetric_plane, and gaussquad
for the axisymmetric one), is registered in src/shapes/index.jl, and exported.
The volume's beta function is evaluated in Arb at each value's own precision and converted
straight back to the shape's real type (the `T(Arblib…(Arb(…); prec))` idiom from
special_functions/factorial.jl) — no BigFloat detour — so Float64 / Double64 / Float128 /
BigFloat / Arb shapes all keep full precision (Float128 uses the type-level
`precision(Float128)` since the instance method is unimplemented upstream).
Validation: volume formulas confirmed by independent 3D Monte-Carlo (rel ~3e-4) and a
per-type precision regression test; degenerate reductions exact (n=1 SuperSpheroidRevolved
== Spheroid via EBCM and Sh to ~1e-8; e=n=1 SuperEllipsoid and n=1 SuperSpheroid reduce to
the ellipsoid/spheroid IITM); n=2 octahedron volume; full suite 48,229 tests pass.
lucifer1004
force-pushed
the
superellipsoid-shapes
branch
from
3edc602 to
fba787b
Compare
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
1 participant
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.
Three new shapes for the Bi Lei group's superquadric dust-aerosol family, with the parameterization verified against Lin & Bi 2018 (JGR, 10.1029/2018JD029464) and Bi et al. 2018 (Opt. Express, 10.1364/OE.26.001726):
SuperSpheroid <: AbstractNFoldShape{4}— |x/a|^(2/n)+|y/a|^(2/n)+|z/c|^(2/n)=1, super-circular cross-sections (D4h, 4-fold; NOT axisymmetric), solved via the n-fold IITM. n=1 spheroid, n=2 octahedron, n>2 concave (dust best-fit n∈[2.4,3.0]).SuperEllipsoid <: AbstractNFoldShape{2}— full triaxial super-ellipsoid with the two roundness exponents (e east-west, n north-south), D2h, via the n-fold IITM.SuperSpheroidRevolved <: AbstractAxisymmetricShape— the true surface of revolution (ϱ/a)^(2/n)+(z/c)^(2/n)=1, so it flows into EBCM / Sh-matrix / near-field; n=1 reduces exactly toSpheroid.Each implements the shape interface (∈, refractive_index, volume/volume_equivalent_radius via the superquadric beta-function formula, rmax, rmin, has_symmetric_plane, and gaussquad for the axisymmetric one), is registered in src/shapes/index.jl, and exported.
The volume's beta function is evaluated in Arb at each value's own precision and converted straight back to the shape's real type (the
T(Arblib…(Arb(…); prec))idiom from special_functions/factorial.jl) — no BigFloat detour — so Float64 / Double64 / Float128 / BigFloat / Arb shapes all keep full precision (Float128 uses the type-levelprecision(Float128)since the instance method is unimplemented upstream).Validation: volume formulas confirmed by independent 3D Monte-Carlo (rel ~3e-4) and a per-type precision regression test; degenerate reductions exact (n=1 SuperSpheroidRevolved == Spheroid via EBCM and Sh to ~1e-8; e=n=1 SuperEllipsoid and n=1 SuperSpheroid reduce to the ellipsoid/spheroid IITM); n=2 octahedron volume; full suite 48,229 tests pass.
Summary by CodeRabbit