#14214: Cythoned homsets
----------------------------------------------+-----------------------------
Reporter: SimonKing | Owner: tbd
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-5.9
Component: performance | Resolution:
Keywords: Hom, cython, cached method | Work issues: Do not change
cmp too much
Report Upstream: N/A | Reviewers:
Authors: Simon King | Merged in:
Dependencies: #14159, #12951 | Stopgaps:
----------------------------------------------+-----------------------------
Comment (by nbruin):
Replying to [comment:18 SimonKing]:
Ah, I see your problems. So the issue is that for `Homset` methods to
function, both `self._domain` and `self._codomain` must be accessible, but
with your patch, neither of these attributes can be assigned or set. In
that sense `Homset` is a kind of "abstract" base class: only subclasses
can be instantiated. I think that is wrong, so in my opinion this puts the
currently proposed solution on the wrong side of positive review. I think
it's really quite a bad hack and we both know that "temporary" solutions
rarely are that.
Perhaps declaring `_domain` and `_codomain` properties rather than cdef
attributes solves the problem? Then at least providing the storage for
those is solved within the class itself. You could use the store that the
cached `domain` and `codomain` provide to store and fetch the value, since
the natural thing -- a `cdef` attribute -- doesn't seem to work due to
layout issues.
The alternative solution is to hack it so that the `Homset` type has
`tp_dictoffset` initialized, if you absolutely want to store `_domain` and
`_codomain` as dynamic attributes. This is something the cython folk have
thought about in response to declaring a `__dict__` attribute, but it
didn't make it in (because it would disable various method lookup
optimizations that we wouldn't have to disable here, if I understand the
discussions there).
> Note, however, that one could get rid of self._domain and
self._codomain. Namely, instead of
> {{{
> self._domain = X
> }}}
> one could do
> {{{
> self.domain.set_cache(X)
> }}}
Right ... with declaring them properties I suspect you could make this
transparent for `_domain` and `_codomain`. Then we can still support them.
> in `Homset.__init__`, and change the cached method into
> {{{
> @cached_method
> def domain(self):
> pass
> }}}
>
> Note that keeping the _domain and _codomain attributes is actually a
cheat:
Yes, thanks for explaining. Unfortunately that makes me really opposed to
the current solution as explained above :-).
> * I would like to make _codomain and _domain cdef attributes, because
this would be faster than a cached method. But this would result in a
layout conflict, as stated in comment:1
I think they ''should'' be cdef attributes because that would allow for
way faster access in cdef-type-declared cython code than `property` access
would.
However, earlier experiments I've done make me hopeful that `property`
access on python level will be fully competitive with cached methods (and
even with python access of `cdef public` attributes, because I'm sure
those are implemented via `property`).
The "proper" solution is to bite the bullet and resolve the layout
conflict, either by code duplication (or can we avoid that via
templating?) or by changing the inheritance structure. I can see why you
don't want to do that on this ticket, but I think we need a better
temporary solution than relying on attributes that can only be
instantiated on a python subclass (which prevents us from having fully
cdef Homset subclasses!)
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14214#comment:19>
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?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.
