Opened 3 years ago
Closed 3 years ago
#23838 closed defect (fixed)
Minor errors in integrated curves
Reported by: | karimvanaelst | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.1 |
Component: | geometry | Keywords: | curve, manifold |
Cc: | egourgoulhon, mbejger, mmancini, tscrim | Merged in: | |
Authors: | Karim Van Aelst | Reviewers: | Eric Gourgoulhon, Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | 0670769 (Commits, GitHub, GitLab) | Commit: | 06707693602218e555a9b2ffb154981e17622beb |
Dependencies: | Stopgaps: |
Description (last modified by )
- In method
solve
of classIntegratedCurve
, the step of integration needs to be evaluated numerically (usingnumerical_approx
) before calling any algorithm, in order to allow it to be an expression such aspi
. - In method
solve
, when using Bulirsch-Stoer algorithm ('bsimp'), methodode_solve
was called in a wrong way. - 'Bulirsch' was misspelled in documentation.
- Minor corrections were made regarding
rk4_maxima
being the default algorithm of methodsolve
.
Change History (9)
comment:1 Changed 3 years ago by
- Branch changed from 41e798a80278ec1a14dba73fabe59009708efe1b to public/manifolds/fix_solve_integrated_curve
- Commit set to 41e798a80278ec1a14dba73fabe59009708efe1b
- Type changed from enhancement to defect
comment:2 Changed 3 years ago by
- Status changed from new to needs_review
comment:3 Changed 3 years ago by
I feel like the fact that the corresponding solve
function does not numerically evaluate such expressions is a bug that deserves its own ticket and (independent) fix. Subsequently, I would remove those changes that work around it.
I cannot comment on the second point, so someone else will have to review that.
comment:4 Changed 3 years ago by
The changes look good to me and I agree with comment:3.
Besides, in the documentation of IntegratedCurve.solve
, shouldn't it be mentioned that the default method (corresponding to method=None
) is rk4_maxima
?
comment:5 follow-up: ↓ 6 Changed 3 years ago by
- Commit changed from 41e798a80278ec1a14dba73fabe59009708efe1b to 06707693602218e555a9b2ffb154981e17622beb
comment:6 in reply to: ↑ 5 ; follow-up: ↓ 8 Changed 3 years ago by
Replying to git:
Thank you for your comments.
Corrections did have to be made about rk4_maxima
being the default algorithm of method solve
, and the numerical evaluation of the step is then maintained until another ticket allows to remove it.
comment:7 Changed 3 years ago by
- Description modified (diff)
comment:8 in reply to: ↑ 6 Changed 3 years ago by
- Reviewers set to Eric Gourgoulhon, Travis Scrimshaw
- Status changed from needs_review to positive_review
Replying to karimvanaelst:
Replying to git:
Thank you for your comments. Corrections did have to be made about
rk4_maxima
being the default algorithm of methodsolve
, and the numerical evaluation of the step is then maintained until another ticket allows to remove it.
Thanks!
comment:9 Changed 3 years ago by
- Branch changed from public/manifolds/fix_solve_integrated_curve to 06707693602218e555a9b2ffb154981e17622beb
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Merge branch 'public/manifolds/point_comparison-23592' of git://trac.sagemath.org/sage into develop
Merge branch 'develop' of https://github.com/sagemath/sage into develop
Merge branch 'public/manifolds/is_trivial_zero' of git://trac.sagemath.org/sage into develop
Fix minor errors in method solve of integrated curves