Opened 9 years ago

Closed 9 years ago

#16280 closed defect (fixed)

Fix call for FiniteEnumeratedSet's of plain Python objects

Reported by: nthiery Owned by:
Priority: major Milestone: sage-6.3
Component: combinatorics Keywords:
Cc: sage-combinat, ncohen, vdelecroix Merged in:
Authors: Nicolas M. Thiéry Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: 4e27454 (Commits, GitHub, GitLab) Commit: 4e274542d24211d57e9f6d99562c56385235721a
Dependencies: Stopgaps:

Status badges

Description (last modified by nthiery)

*Problem*

Before this ticket, a FiniteEnumeratedSet could not be called on its own elements whenever they were not Element's:

sage: F = FiniteEnumeratedSet(["a", 1])
sage: F(1)
1
sage: F("a")
...
TypeError: Cannot convert str to sage.structure.element.Element

This prevented the use of F(x) as generic idiom to convert x into F while checking that it's in F.

And indeed:

sage: TestSuite(F).run()
Failure in _test_an_element:
...
TypeError: Cannot convert str to sage.structure.element.Element
------------------------------------------------------------
The following tests failed: _test_an_element

*Analysis*

Parent.__call__ enforces that _element_constructor_ return an Element (more precisely, it calls _element_constructor_ through a DefaultConvertMap, and any Map requires its results to be instances of Element).

*Proposed solution*

Since FiniteEnumeratedSets is often a facade over plain Python objects, this ticket works around this limitation by a custom FiniteEnumeratedSets.__call__ that calls directly _element_constructor_ whenever el is not an Element. Otherwise Parent.__call__ is called as usual.

*Limitation*

This workaround prevents conversions or coercions from facade parents over plain Python objects. But it's already much better than before!

Change History (16)

comment:1 Changed 9 years ago by nthiery

Authors: Nicolas M. Thiéry
Cc: sage-combinat ncohen vdelecroix added
Component: PLEASE CHANGEcombinatorics
Description: modified (diff)
Type: PLEASE CHANGEdefect

comment:2 Changed 9 years ago by nthiery

Summary: Fix FiniteEnumeratedSet's call to accept Python objects as inputFix call for FiniteEnumeratedSet over plain Python objects

comment:3 Changed 9 years ago by nthiery

Branch: u/nthiery/fix_call_for_finite_enumeratedset_s_over_plain_python_objects

comment:4 Changed 9 years ago by nthiery

Commit: 8ac32c2d6854519a96eb1ce261240a0ee8537cb3
Description: modified (diff)
Status: newneeds_review
Summary: Fix call for FiniteEnumeratedSet over plain Python objectsFix call for FiniteEnumeratedSet's of plain Python objects

New commits:

8ac32c2#16280: Fix call for FiniteEnumeratedSet's of plain Python objects

comment:5 Changed 9 years ago by ncohen

sage -t --long sets/totally_ordered_finite_set.py  # 1 doctest failed
sage -t --long sets/finite_enumerated_set.py  # 3 doctests failed

Nothing serious, but there is something weird O_O;;

File "sets/finite_enumerated_set.py", line 345, in sage.sets.finite_enumerated_set.FiniteEnumeratedSet.__call__
Failed example:
    psi('a')
Expected:
    2
Got:
    1

comment:6 Changed 9 years ago by ncohen

Status: needs_reviewneeds_work

comment:7 Changed 9 years ago by ncohen

Oh, I did not try to pass test in all of src/. My office's computer is off for some reason ^^;

Nathann

comment:8 Changed 9 years ago by git

Commit: 8ac32c2d6854519a96eb1ce261240a0ee8537cb34e274542d24211d57e9f6d99562c56385235721a

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

4e27454#16280: Trivial doctest fixes

comment:9 Changed 9 years ago by nthiery

Running all long tests now. I'll report around noon.

comment:10 Changed 9 years ago by ncohen

Do you get why this 2 changed to a 1 ? O_o

comment:11 in reply to:  10 Changed 9 years ago by nthiery

Replying to ncohen:

Do you get why this 2 changed to a 1 ? O_o

It was meant to be a one in the first place: it's the length of 'a'; I had fumbled my copy-paste.

Btw: all long tests pass.

comment:12 Changed 9 years ago by nthiery

Status: needs_workneeds_review

comment:13 Changed 9 years ago by ncohen

Oh sorry, I had not noticed that you had added this doctest with '2' as an answer yourself. I was wondering how your commit could have changed a former "2" into a "1" :-D

Good to go as far as I can tell. Seems to do what it should, and if it passes tests... Thanks ! Now #16269 can be reviewed, which means that #16277 can be reviewed which means that #16279 can be reviwed, but also #16235 and so #16241.

Damn designs :-PPPPPPPPP

Nathann

comment:14 Changed 9 years ago by ncohen

Reviewers: Nathann Cohen
Status: needs_reviewpositive_review

comment:15 Changed 9 years ago by vbraun_spam

Milestone: sage-6.2sage-6.3

comment:16 Changed 9 years ago by vbraun

Branch: u/nthiery/fix_call_for_finite_enumeratedset_s_over_plain_python_objects4e274542d24211d57e9f6d99562c56385235721a
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.