Opened 10 years ago

Closed 10 years ago

#10995 closed defect (fixed)

no checking on getitem for Ambient space of a root system

Reported by: VivianePons Owned by: sage-combinat
Priority: critical Milestone: sage-4.7
Component: combinatorics Keywords: ambient space, root system, getitem, combinatorialfreemodule
Cc: sage-combinat Merged in: sage-4.7.alpha3
Authors: Viviane Pons Reviewers: Nicolas M. Thiéry
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

When you create an ambient space of a root system, there is a getitem method. But no verification is made on the given value which implies bugs on other uses of the object.

sage: R = RootSystem('A2')
sage: A2 = R.ambient_space()
sage: A2[0]
(0, 0, 0)
sage: list(A2[0])
[(-1, 1)]
sage: A2.zero()
(0, 0, 0)
sage: list(A2.zero())
[]

The getitem should only get values between 1 and 3 and raise exceptions when not. Otherwise, it allows for creating elements which don't make any sense and can bring trouble :

sage: C = CombinatorialFreeModule(QQ,A2)
sage: C.an_element()
B[(0, 0, 0)] + 2*B[(1, 0, 0)] + B[(2, 2, 3)] + 3*B[(0, 1, 0)]
sage: elt = C(A2([0,0,0])) + 2 * C(A2([1,0,0])) + C(A2([2,2,3])) + 3 * C(A2([0,1,0]))
sage: elt
B[(0, 0, 0)] + 2*B[(1, 0, 0)] + B[(2, 2, 3)] + 3*B[(0, 1, 0)]
sage: elt == C.an_element()
False

Indeed, the getitem is used by python to iterate on the parent and the iteration is used by CombinatorialFreeModule? to create elements.

Attachments (1)

trac_10995-fixing_getitem_on_ambient_space-vp.patch (917 bytes) - added by VivianePons 10 years ago.
the patch

Download all attachments as: .zip

Change History (9)

comment:1 Changed 10 years ago by VivianePons

  • Status changed from new to needs_review

comment:2 Changed 10 years ago by nthiery

  • Cc sage-combinat added
  • Reviewers set to Nicolas M. Thiéry
  • Status changed from needs_review to positive_review

Following the discussion on

http://groups.google.com/group/sage-combinat-devel/browse_thread/thread/ab1722fb97c0fbbb

this is good to go (assuming the buildbot goes green which I expect)!

Thanks,

Nicolas

comment:3 Changed 10 years ago by jdemeyer

  • Milestone set to sage-4.7

comment:4 follow-up: Changed 10 years ago by jdemeyer

  • Status changed from positive_review to needs_work

@VivianePons?: create a .hgrc file as explained in http://sagemath.org/doc/developer/walk_through.html#submitting-a-change and resubmit the patch.

comment:5 in reply to: ↑ 4 Changed 10 years ago by nthiery

Replying to jdemeyer:

@VivianePons?: create a .hgrc file as explained in http://sagemath.org/doc/developer/walk_through.html#submitting-a-change and resubmit the patch.

Sorry, I should have caught this. The main point is that the patch should be exported before being posted on trac. See also: http://wiki.sagemath.org/combinat/MercurialStepByStep#ExportingPatchesforusewithtrac.

Changed 10 years ago by VivianePons

the patch

comment:6 Changed 10 years ago by VivianePons

  • Status changed from needs_work to needs_review

comment:7 Changed 10 years ago by nthiery

  • Status changed from needs_review to positive_review

comment:8 Changed 10 years ago by jdemeyer

  • Merged in set to sage-4.7.alpha3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.