Opened 7 months ago

Closed 6 months ago

#29639 closed defect (fixed)

Bug in initialization of vector field with rational components

Reported by: egourgoulhon Owned by:
Priority: major Milestone: sage-9.2
Component: geometry Keywords: vector field, manifold
Cc: tscrim Merged in:
Authors: Eric Gourgoulhon Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 62dc948 (Commits) Commit: 62dc948cc2fb4309af1cfa4b984e377f31852acb
Dependencies: Stopgaps:

Description (last modified by egourgoulhon)

Initializing the components of a vector field with a rational number as first component leads to an error:

sage: M = Manifold(2, 'M')
sage: X.<x,y> = M.chart()
sage: v = M.vector_field(1/2, -1)
...
IndexError: index n (=1) out of range; it must be 0

while providing the components as a list is OK:

sage: v = M.vector_field([1/2, -1])
sage: v.display()
1/2 d/dx - d/dy

This is due to Rational class having a __getitem__() method (for some reason...) and TensorField._init_components (introduced in #27581) testing its input by

    if hasattr(comp0, '__getitem__'):

Replacing the above line by

    if hasattr(comp0, '__len__') and hasattr(comp0, '__getitem__'):

fixes the bug, since Rational has no __len__ method.

Change History (9)

comment:1 Changed 7 months ago by egourgoulhon

  • Branch set to public/manifolds/bug_init_vector_field-29639
  • Commit set to 3142781e671de21bf5e242a823fbe8c0a70fb72a

New commits:

3142781Fix bug on initializing a vector field with rational components (trac #29639)

comment:2 Changed 7 months ago by egourgoulhon

  • Cc tscrim added
  • Status changed from new to needs_review

comment:3 Changed 7 months ago by egourgoulhon

  • Description modified (diff)

comment:4 Changed 7 months ago by git

  • Commit changed from 3142781e671de21bf5e242a823fbe8c0a70fb72a to 62dc948cc2fb4309af1cfa4b984e377f31852acb

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

62dc948#29639: correct the fix

comment:5 Changed 7 months ago by egourgoulhon

  • Description modified (diff)

comment:6 Changed 7 months ago by egourgoulhon

The fix proposed in comment:1, namely use of

    if isinstance(comp0, (list, tuple)):

failed if comp0 is an object created by vector(...), as revealed by a failed doctest in vectorfield.py. If the latest commit (comment:4), this is replaced by

    if hasattr(comp0, '__len__') and hasattr(comp0, '__getitem__'):

This returns True (as it should) for list, tuple and vector and False (as it should) for a Rational.

Version 0, edited 7 months ago by egourgoulhon (next)

comment:7 follow-up: Changed 7 months ago by tscrim

  • Milestone changed from sage-9.1 to sage-9.2
  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

This will work. (Note that matrices also fall under this criteria, but I don't know of a (reasonable) field that would. Number fields, p-adics, polynomials, etc. have a __getitem__ but not a __len__.) Generally I would prefer something more explicit, but I am okay with the generality.

comment:8 in reply to: ↑ 7 Changed 7 months ago by egourgoulhon

Replying to tscrim:

This will work. (Note that matrices also fall under this criteria,

That's a good point; it's nice that it works for matrices too.

but I don't know of a (reasonable) field that would. Number fields, p-adics, polynomials, etc. have a __getitem__ but not a __len__.) Generally I would prefer something more explicit,

Me too, but I could not think of something better in that case...

but I am okay with the generality.

Thank you for the review!

comment:9 Changed 6 months ago by vbraun

  • Branch changed from public/manifolds/bug_init_vector_field-29639 to 62dc948cc2fb4309af1cfa4b984e377f31852acb
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.