Opened 4 years ago

Last modified 4 years ago

#23838 closed defect

Minor errors in integrated curves — at Version 7

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:
Report Upstream: N/A Work issues:
Branch: public/manifolds/fix_solve_integrated_curve (Commits, GitHub, GitLab) Commit: 06707693602218e555a9b2ffb154981e17622beb
Dependencies: Stopgaps:

Status badges

Description (last modified by karimvanaelst)

  • In method solve of class IntegratedCurve, the step of integration needs to be evaluated numerically (using numerical_approx) before calling any algorithm, in order to allow it to be an expression such as pi.
  • In method solve, when using Bulirsch-Stoer algorithm ('bsimp'), method ode_solve was called in a wrong way.
  • 'Bulirsch' was misspelled in documentation.
  • Minor corrections were made regarding rk4_maxima being the default algorithm of method solve.

Change History (7)

comment:1 Changed 4 years ago by karimvanaelst

  • Branch changed from 41e798a80278ec1a14dba73fabe59009708efe1b to public/manifolds/fix_solve_integrated_curve
  • Commit set to 41e798a80278ec1a14dba73fabe59009708efe1b
  • Type changed from enhancement to defect

New commits:

6c66fd9Merge branch 'public/manifolds/point_comparison-23592' of git:// into develop
3b65b82Merge branch 'develop' of into develop
7359158Merge branch 'public/manifolds/is_trivial_zero' of git:// into develop
41e798aFix minor errors in method solve of integrated curves

comment:2 Changed 4 years ago by karimvanaelst

  • Status changed from new to needs_review

comment:3 Changed 4 years ago by tscrim

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 4 years ago by egourgoulhon

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: Changed 4 years ago by git

  • Commit changed from 41e798a80278ec1a14dba73fabe59009708efe1b to 06707693602218e555a9b2ffb154981e17622beb

Branch pushed to git repo; I updated commit sha1. New commits:

02e92a3Merge branch 'develop' of into develop to get latest development sources.
0670769Corrections regarding 'rk4_maxima' being the default algorithm of method 'solve'

comment:6 in reply to: ↑ 5 Changed 4 years ago by karimvanaelst

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 4 years ago by karimvanaelst

  • Description modified (diff)
Note: See TracTickets for help on using tickets.