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:

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 (9)

comment:1 Changed 3 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://trac.sagemath.org/sage into develop
​3b65b82Merge branch 'develop' of https://github.com/sagemath/sage into develop
​7359158Merge branch 'public/manifolds/is_trivial_zero' of git://trac.sagemath.org/sage into develop
​41e798aFix minor errors in method solve of integrated curves

comment:2 Changed 3 years ago by karimvanaelst

  • Status changed from new to needs_review

comment:3 Changed 3 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 3 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 3 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 https://github.com/sagemath/sage into develop to get latest development sources.
​0670769Corrections regarding 'rk4_maxima' being the default algorithm of method 'solve'

comment:6 in reply to: ↑ 5 ; follow-up: Changed 3 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 3 years ago by karimvanaelst

  • Description modified (diff)

comment:8 in reply to: ↑ 6 Changed 3 years ago by egourgoulhon

  • 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 method solve, 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 vbraun

  • Branch changed from public/manifolds/fix_solve_integrated_curve to 06707693602218e555a9b2ffb154981e17622beb
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.