Opened 5 years ago

Closed 5 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) Commit: 4e274542d24211d57e9f6d99562c56385235721a
Dependencies: Stopgaps:

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 5 years ago by nthiery

  • Authors set to Nicolas M. Thiéry
  • Cc sage-combinat ncohen vdelecroix added
  • Component changed from PLEASE CHANGE to combinatorics
  • Description modified (diff)
  • Type changed from PLEASE CHANGE to defect

comment:2 Changed 5 years ago by nthiery

  • Summary changed from Fix FiniteEnumeratedSet's call to accept Python objects as input to Fix call for FiniteEnumeratedSet over plain Python objects

comment:3 Changed 5 years ago by nthiery

  • Branch set to u/nthiery/fix_call_for_finite_enumeratedset_s_over_plain_python_objects

comment:4 Changed 5 years ago by nthiery

  • Commit set to 8ac32c2d6854519a96eb1ce261240a0ee8537cb3
  • Description modified (diff)
  • Status changed from new to needs_review
  • Summary changed from Fix call for FiniteEnumeratedSet over plain Python objects to Fix call for FiniteEnumeratedSet's of plain Python objects

New commits:

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

comment:5 Changed 5 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 5 years ago by ncohen

  • Status changed from needs_review to needs_work

comment:7 Changed 5 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 5 years ago by git

  • Commit changed from 8ac32c2d6854519a96eb1ce261240a0ee8537cb3 to 4e274542d24211d57e9f6d99562c56385235721a

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

4e27454#16280: Trivial doctest fixes

comment:9 Changed 5 years ago by nthiery

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

comment:10 follow-up: Changed 5 years ago by ncohen

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

comment:11 in reply to: ↑ 10 Changed 5 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 5 years ago by nthiery

  • Status changed from needs_work to needs_review

comment:13 Changed 5 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 5 years ago by ncohen

  • Reviewers set to Nathann Cohen
  • Status changed from needs_review to positive_review

comment:15 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:16 Changed 5 years ago by vbraun

  • Branch changed from u/nthiery/fix_call_for_finite_enumeratedset_s_over_plain_python_objects to 4e274542d24211d57e9f6d99562c56385235721a
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.