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

Cannot subclass? #40

Closed
pombredanne opened this issue May 4, 2016 · 10 comments
Closed

Cannot subclass? #40

pombredanne opened this issue May 4, 2016 · 10 comments
Assignees

Comments

@pombredanne
Copy link
Collaborator

pombredanne commented May 4, 2016

Hi:
I am trying to create a simple subclass of intbitset without luck:

>>> from intbitset import intbitset
>>> class MySet(intbitset):
...  def __init__(a1, a2):
...   super(MySet, self).__init__(range(a1, a2))
... 
>>> MySet(1, 10)
Exception TypeError: '__del__() takes exactly one argument (0 given)' in <built-in method __del__ of MySet object at 0x7f21724b9158> ignored
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: __init__() takes exactly 2 arguments (3 given)

>>> class MySet(intbitset):
...  def __new__(cls, *args):
...   return intbitset.__new__(cls, args)
... 
>>> MySet([1, 10])
intbitset([1])
>>> MySet(1, 10)
Exception TypeError: '__del__() takes exactly one argument (0 given)' in <built-in method __del__ of MySet object at 0x7f21724b9260> ignored
intbitset([1, 10])

and then in the same session as above:

>>> a=intbitset([1,2,5])
>>> a=intbitset([1,2,5,1000])
>>> a=intbitset([1,2,5])
>>> b=intbitset([1,2,5,1000])
>>> d={a: 12}
>>> d
Exception TypeError: '__del__() takes exactly one argument (0 given)' in <built-in method __del__ of MySet object at 0x7f21724b92b8> ignored
{intbitset([1, 2, 5]): 12}
@kaplun
Copy link
Member

kaplun commented May 4, 2016

Hi @pombredanne,

given that intbitset is implemented as C extension I am not sure about its exstensibility out of the box. I would need to investigate a bit.

kaplun added a commit to kaplun/intbitset that referenced this issue May 4, 2016
* FIX Fixes implementation of del x[123] operator which was wrongly
  defined as __del__ rathern than __delitem__.
  (Closes inveniosoftware-contrib#40)

Signed-off-by: Samuele Kaplun <[email protected]>
Reported-by: Philippe Ombredanne <[email protected]>
@kaplun kaplun mentioned this issue May 4, 2016
@kaplun
Copy link
Member

kaplun commented May 4, 2016

Dear @pombredanne, the proposed PR should fix the __del__() issue. Regarding the proper creation of a subclass, intbitset is special in that all its initialization happens in the __cinit__() special method that is not accessible in Python.

So to implement your initialization you could for example write:

from intbitset import intbitset
class MySet(intbitset):
    def __init__(self, a1, a2):
        # self is already an initalized empty intbitset
        self.union_update(range(a1, a2))

kaplun added a commit to kaplun/intbitset that referenced this issue May 4, 2016
* FIX Fixes implementation of `del x[123]` operator which was wrongly
  defined as `__del__` rathern than `__delitem__`.
  (closes inveniosoftware-contrib#40)

Signed-off-by: Samuele Kaplun <[email protected]>
Reported-by: Philippe Ombredanne <[email protected]>
kaplun added a commit to kaplun/intbitset that referenced this issue May 4, 2016
* FIX Fixes implementation of `del x[123]` operator which was wrongly
  defined as `__del__` rathern than `__delitem__`.
  (closes inveniosoftware-contrib#40)

Signed-off-by: Samuele Kaplun <[email protected]>
Reported-by: Philippe Ombredanne <[email protected]>
kaplun added a commit to kaplun/intbitset that referenced this issue May 4, 2016
* FIX Fixes implementation of `del x[123]` operator which was wrongly
  defined as `__del__` rather than `__delitem__`.
  (closes inveniosoftware-contrib#40)

Signed-off-by: Samuele Kaplun <[email protected]>
Reported-by: Philippe Ombredanne <[email protected]>
@kaplun
Copy link
Member

kaplun commented May 4, 2016

Ah additionally:

>> a=intbitset([1,2,5])
>> d={a: 12}
>> d
{intbitset([1, 2, 5]): 12}

This will not always work because the hash function of intbitset is not properly implemented. See also: #15. I'd discurage to use an intbitset as a key on a dictionary (beside intbitset is mutable as set and list for which this pattern is not even allowed:

In [1]: {set([1,2,3]): 1}
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-1-d01249751226> in <module>()
----> 1 {set([1,2,3]): 1}

TypeError: unhashable type: 'set'

See also: #16

@pombredanne
Copy link
Collaborator Author

@kaplun Thanks ++
and FWIW I do not intend to make intbitset hashable, but rather have a hashable immutable subclass.

But there is still something not right though IMHO:

>>> class MySet(intbitset):
...     def __init__(self, a1, a2):
...         self.union_update(range(a1, a2))
... 
>>> a=MySet(0, 12)
>>> b=MySet(5, 15)
>>> a|b
intbitset([0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14])

I would expect a MySet object instead of an intbitset.

For instance with a set:

class SubSet(set):
    def __init__(self, a1, a2):
        super(SubSet, self).__init__(range(a1, a2))

I get the right subclass:

>>> a=SubSet(0, 12)
>>> b=SubSet(5, 15)
>>> a|b
SubSet([0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14])

@kaplun
Copy link
Member

kaplun commented May 4, 2016

That's probably due to the fact that the intbitset method and operators explicitly return a new intbitset object (I never thought about the use case of inheritance 😄 but was more focused on implementing a C extension). I think though that it should be possible to implement what you are looking for. I'll investigate further. (Although all this runtime type checking is going to reduce a bit the performance of the module, where all the types are currently made explicit at compile time).

@pombredanne
Copy link
Collaborator Author

@kaplun hold that thought then. I may able to get the exact same results by delegation rather than inheritance.

@pombredanne
Copy link
Collaborator Author

FWIW, you may want to reopen this

@kaplun
Copy link
Member

kaplun commented May 5, 2016

Do you mean WRT operators returning explicitly intbitset or there is still a proble with __del__?

@pombredanne
Copy link
Collaborator Author

pombredanne commented May 29, 2016

Actually intbitset seem to be kinda like an immutable structure when it comes to subclassing and therefore closer to a frozenset than a set. Hence overriding __init__ does not work as intented here, but overriding __new__ and invoking super works fine. By the time __init__ is called, an object instance has already been created so it's too late

@pombredanne
Copy link
Collaborator Author

pombredanne commented May 29, 2016

For instance this works nicely...

>>> class MySet(intbitset):
...  def __new__(cls, x , y):
...   return super(MySet, cls).__new__(cls, range(x, y + 1))
...  def __repr__(self):
...   return 'Myset(%r)' % sorted(self)
... 
>>> MySet(1,5)
Myset([1, 2, 3, 4, 5])
>>> type(MySet(1,5))
<class '__main__.MySet'>

well I still need to override union though

>>> MySet(1,5)| MySet(4,12)
intbitset([1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12])

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

3 participants