Opened 2 years ago
Closed 2 years 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, GitHub, GitLab) | Commit: | 62dc948cc2fb4309af1cfa4b984e377f31852acb |
Dependencies: | Stopgaps: |
Description (last modified by )
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 2 years ago by
- Branch set to public/manifolds/bug_init_vector_field-29639
- Commit set to 3142781e671de21bf5e242a823fbe8c0a70fb72a
comment:2 Changed 2 years ago by
- Cc tscrim added
- Status changed from new to needs_review
comment:3 Changed 2 years ago by
- Description modified (diff)
comment:4 Changed 2 years ago by
- 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 2 years ago by
- Description modified (diff)
comment:6 Changed 2 years ago by
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
.
comment:7 follow-up: ↓ 8 Changed 2 years ago by
- 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 2 years ago by
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 2 years ago by
- Branch changed from public/manifolds/bug_init_vector_field-29639 to 62dc948cc2fb4309af1cfa4b984e377f31852acb
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Fix bug on initializing a vector field with rational components (trac #29639)