Opened 7 months ago

Closed 7 months ago

#28669 closed defect (fixed)

Unnecessary coordinate check while initializing an integrated curve

Reported by: egourgoulhon Owned by:
Priority: major Milestone: sage-9.0
Component: geometry Keywords: integrated_curve, geodesic
Cc: karimvanaelst Merged in:
Authors: Eric Gourgoulhon Reviewers: Karim Van Aelst
Report Upstream: N/A Work issues:
Branch: c0efb2e (Commits) Commit: c0efb2e0e181bbc29818736c69d68648604a7d7a
Dependencies: Stopgaps:

Description

In Sage 8.9, we have:

sage: E.<r,phi> = EuclideanSpace(coordinates='polar')
sage: p = E((1, 0))  # the initial point
sage: v = E.tangent_space(p)((2, 1))  # the initial vector
sage: t = var('t')
sage: c = E.integrated_geodesic(E.metric(), (t, 0, 10), v)
...
ValueError: initial point should be in the domain of the chart

Now, the initial point p does lie in the domain of the chart:

sage: E.default_chart()(p)
(1, 0)

Indeed, 0 is a perfectly valid value for the polar coordinate phi:

sage: E.default_chart().coord_range()
r: (0, +oo); phi: [0, 2*pi] (periodic)

The above error turns out to result from a too strict and unnecessary coordinate check performed in IntegratedCurve.__init__(). This check is removed here.

Change History (9)

comment:1 Changed 7 months ago by egourgoulhon

  • Branch set to public/manifolds/integrated_curve_init
  • Cc karimvanaelst added
  • Commit set to 3ab55d7e91044c0cb177f9bfd125f30c281bf3da
  • Status changed from new to needs_review

New commits:

3ab55d7Remove unnecessary coordinate check in IntegratedCurve.__init__()

comment:2 follow-up: Changed 7 months ago by chapoton

please add a doctest

comment:3 Changed 7 months ago by git

  • Commit changed from 3ab55d7e91044c0cb177f9bfd125f30c281bf3da to c0efb2e0e181bbc29818736c69d68648604a7d7a

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

c0efb2eAdd doctest in IntegratedCurve.__init__()

comment:4 in reply to: ↑ 2 ; follow-up: Changed 7 months ago by egourgoulhon

Replying to chapoton:

please add a doctest

Done in the above commit.

comment:5 in reply to: ↑ 4 Changed 7 months ago by karimvanaelst

  • Status changed from needs_review to positive_review

Replying to egourgoulhon:

Replying to chapoton:

please add a doctest

Done in the above commit.

Ticket seems ready to be closed.

comment:6 Changed 7 months ago by egourgoulhon

Thanks for the review!

comment:7 Changed 7 months ago by vbraun

  • Status changed from positive_review to needs_work

Reviewer name is missing

comment:8 Changed 7 months ago by karimvanaelst

  • Reviewers set to Karim Van Aelst
  • Status changed from needs_work to positive_review

comment:9 Changed 7 months ago by vbraun

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