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

FiniteEnumeratedSet creates elements that don't inherit from Element #12048

Closed
roed314 opened this issue Nov 17, 2011 · 14 comments
Closed

FiniteEnumeratedSet creates elements that don't inherit from Element #12048

roed314 opened this issue Nov 17, 2011 · 14 comments

Comments

@roed314
Copy link
Contributor

roed314 commented Nov 17, 2011

    sage: S = FiniteEnumeratedSet(['a','b','c'])
    sage: S('a')
    ...
    TypeError: Cannot convert str to sage.structure.element.Element

Fixed by #16280

CC: @hivert @kini @videlec

Component: combinatorics

Reviewer: Florent Hivert

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

@roed314
Copy link
Contributor Author

roed314 commented Nov 17, 2011

comment:1

Attachment: 12048.patch.gz

@nthiery
Copy link
Contributor

nthiery commented Nov 17, 2011

comment:2

Hi David,

I agree that S('a') in the example above should not fail. On the other hand, I am not keen on having a facade parent touching its elements. Florent: we should discuss about that.

@roed314
Copy link
Contributor Author

roed314 commented Nov 17, 2011

comment:3

Yeah, I wasn't completely happy with this solution either. But I think that even though the elements of a facade parent don't have to have the right parent, they really should be Elements.

I'd be happy to entertain other solutions.

@roed314
Copy link
Contributor Author

roed314 commented Nov 19, 2011

comment:4

I'm not going to debug this now since you guys may want to change the approach, but this patch produces doctest errors in sage.combinat.free_module that seem to be related to sorting.

@loefflerd loefflerd mannequin added s: needs work and removed s: needs review labels Mar 11, 2012
@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Mar 12, 2012

comment:6

Any movement here? This has ended up being a prerequisite for a couple of other patches (#12039, #12260) and hence a lot of good code is piling up behind this.

I had a look at the error this causes in combinat.free_module, but I have no idea what's going on there.

@hivert
Copy link

hivert commented Mar 17, 2012

comment:7

Replying to @loefflerd:

Any movement here? This has ended up being a prerequisite for a couple of other patches (#12039, #12260) and hence a lot of good code is piling up behind this.

I had a look at the error this causes in combinat.free_module, but I have no idea what's going on there.

Sorry that we forgot this one. I'll try to discuss it with Nicolas shortly.

Florent

@nthiery
Copy link
Contributor

nthiery commented Jun 27, 2012

comment:9

Dear David,

Sorry for lagging so much behind on this ticket. I have finally been
discussing with Florent, and we agreed on the following:

  • We definitely want to have facade parents in Sage whose elements
    are plain Python objects. In fact we already have some: you can
    e.g. define facade posets on strings or
    sage.structure.parent.Set_PythonType(int).

    Under many circumstances, wrapping objects sure has its advantages
    (the element can make use of the parent information for certain
    operations), but there are also many use cases where constantly
    wrapping/unwrapping is a pain.

    The worst is when a user gives a list of objects as input to
    construct some parent A (e.g. a CombinatorialFreeModule), and then
    (s)he accesses those objects from A he gets something that looks
    and smell like the original objects but differ in behavior in a
    subtle way

  • Coercion should work for those parents. It does not currently due
    to this test whether the end result of __call__ is an Element. This
    should be easy to fix. We already got hurt by that with Posets, and
    had to resort to a hand made __call__ (yuck).

  • By default, we prefer FiniteEnumeratedSets to be facades by default
    to not break backward compatibility, and also because this seems to
    be what we need most of the time (because there are barely no
    operations on the elements).

. Now if there is a clear need for it, we could add a facade
option to have non facade FiniteEnumeratedSets.

What do you think? Do you feel like fixing coercion as above?

Cheers,
Nicolas

@jdemeyer jdemeyer modified the milestones: sage-5.11, sage-5.12 Aug 13, 2013
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.1, sage-6.2 Jan 30, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.2, sage-6.3 May 6, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.3, sage-6.4 Aug 10, 2014
@mezzarobba
Copy link
Member

comment:15

Is this ticket still relevant? The example in the description now seems to work.

@videlec

This comment has been minimized.

@videlec
Copy link
Contributor

videlec commented Mar 23, 2018

Changed author from David Roe to none

@videlec
Copy link
Contributor

videlec commented Mar 23, 2018

comment:16

Indeed a custom __call__ was introduced to support this #16280.

@videlec videlec removed this from the sage-6.4 milestone Mar 23, 2018
@hivert
Copy link

hivert commented Mar 24, 2018

comment:17

I confirm that this is a dup of #16280.

@hivert
Copy link

hivert commented Mar 24, 2018

Reviewer: Florent Hivert

@videlec
Copy link
Contributor

videlec commented May 18, 2018

comment:18

closing positively reviewed duplicates

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

6 participants