Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
nthiery opened this issue May 2, 2014 · 16 comments
Closed

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

nthiery opened this issue May 2, 2014 · 16 comments

Comments

@nthiery
Copy link
Contributor

nthiery commented May 2, 2014

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!

CC: @sagetrac-sage-combinat @nathanncohen @videlec

Component: combinatorics

Author: Nicolas M. Thiéry

Branch/Commit: 4e27454

Reviewer: Nathann Cohen

Issue created by migration from https://trac.sagemath.org/ticket/16280

@nthiery nthiery added this to the sage-6.2 milestone May 2, 2014
@nthiery

This comment has been minimized.

@nthiery
Copy link
Contributor Author

nthiery commented May 2, 2014

Author: Nicolas M. Thiéry

@nthiery nthiery changed the title Fix FiniteEnumeratedSet's call to accept Python objects as input Fix call for FiniteEnumeratedSet over plain Python objects May 2, 2014
@nthiery
Copy link
Contributor Author

nthiery commented May 2, 2014

@nthiery
Copy link
Contributor Author

nthiery commented May 2, 2014

Commit: 8ac32c2

@nthiery
Copy link
Contributor Author

nthiery commented May 2, 2014

New commits:

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

@nthiery

This comment has been minimized.

@nthiery nthiery changed the title Fix call for FiniteEnumeratedSet over plain Python objects Fix call for FiniteEnumeratedSet's of plain Python objects May 2, 2014
@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented May 3, 2014

comment:5
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

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented May 3, 2014

comment:7

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

Nathann

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 3, 2014

Changed commit from 8ac32c2 to 4e27454

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 3, 2014

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

4e27454#16280: Trivial doctest fixes

@nthiery
Copy link
Contributor Author

nthiery commented May 3, 2014

comment:9

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

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented May 3, 2014

comment:10

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

@nthiery
Copy link
Contributor Author

nthiery commented May 3, 2014

comment:11

Replying to @nathanncohen:

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.

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented May 3, 2014

comment:13

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

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented May 3, 2014

Reviewer: Nathann Cohen

@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.2, sage-6.3 May 6, 2014
@vbraun
Copy link
Member

vbraun commented May 6, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants