#15367: Empty lists while creating parents
-------------------------------------+-------------------------------------
Reporter: roed | Owner:
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-5.13
Component: coercion | Resolution:
Keywords: | Merged in:
Authors: Nils Bruin | Reviewers: Simon King
Report Upstream: N/A | Work issues:
Branch: | Commit:
u/nbruin/ticket/15367 | cce61195a94ce70f27c0a3efeafbccf4f190f5f0
Dependencies: | Stopgaps:
-------------------------------------+-------------------------------------
Comment (by nbruin):
Replying to [comment:32 SimonKing]:
> Can you elaborate: What is `PyCapsule`? I've never heard of it.
It's a python object wrapping a `void *`:
http://docs.python.org/2/c-api/capsule.html , so it does exactly what we
need. In cython it would be something along the lines of:
{{{
cdef class myPyCapsule:
cdef void* pointer
}}}
I don't think doing that would be much faster, though: we'd be receiving a
`<object>` that we have to cast to a !myPyCapsule, so cython would be
generating the type testing code anyway, which is probably what
!PyCapsule_GetPointer is spending most of its time on anyway. So going
with the standard python solution for the thing seems appropriate.
Previously, we were casting `<void*>` to `<Py_ssize_t>` which we then
wrapped as a `PyInt` object. `PyCapsule` seems more appropriate. (in our
previous implementation it wasn't, because our buckets we `PyList`, so our
keys weren't stored as bare `void*` anyway. I don't think `PyCapsule`
objects have sensible comparison semantics, so packaging them as `PyInt`
made more sense.
> Would it make sense to have `cdef type fixed_KeyedRef=KeyedRef` instead
of just `cdef fixed_KeyedRef=KeyedRef`? After all, we do "isinstance" for
a couple of times, so, it might help Cython if it knows that `KeyedRef`
actually ''is'' a type.
I suspect for creation cython generates a `PyObject_callobject` anyway, so
there it shouldn't matter. !KeyedRef is implemented in python anyway, so
there's no sense in trying to do anything smarter there anyway.
For the typecheck it might make a difference, but I expect we have to
nudge cython on that by explicitly using `PyObject_TypeCheck`. I'm pretty
sure cython's `isinstance` will just translate into `PyObject_IsInstance`
(or perhaps it really just goes off and look up the function in the
builtin dict). And that actually does happen in the more critical bits of
the code! It's an easy change to make. I'll push it later.
> Concerning the hash function: Didn't we have the current hash function
`<size_t>key1 + 13*(<size_t>key2) ^ 503*(<size_t>key3)` in the original
implementation? If I recall correctly, I chose it because it was
recommended as a good hash function for tuples.
Why did you (temporarily?) change it to `<size_t>key1 + (<size_t>key2)<<2
+ (<size_t>key3)<<4`?
It is indeed a good hash function, particularly for moduli that are a
power of 2. Where did you get it from? or was it already in the code?
My change was just misguided. I thought the other hash was based on "prime
modulus", so I wasn't assuming it was particularly good for my situation.
And I thought the new code wouldn't be so sensitive, because it still has
the "perturb" as well. I was wrong.
--
Ticket URL: <http://trac.sagemath.org/ticket/15367#comment:33>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/groups/opt_out.
