Opened 4 years ago
Closed 4 years ago
#25134 closed enhancement (fixed)
Derivation and twisted derivations over rings
Reported by: | gh-Diarmund33 | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.4 |
Component: | algebra | Keywords: | rings, derivation |
Cc: | caruso, jsrn, tscrim | Merged in: | |
Authors: | Amaury Durand, Xavier Caruso | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | c8fee99 (Commits, GitHub, GitLab) | Commit: | c8fee99e5ac2c4148b885cb33367d4b64da7822c |
Dependencies: | #26354 | Stopgaps: |
Description (last modified by )
This ticket implements derivation and theta
-derivations over rings.
(It would be useful for implementing general Ore polynomials.)
Change History (44)
comment:1 Changed 4 years ago by
- Branch set to u/adurand/derivation
comment:2 Changed 4 years ago by
- Commit set to b927acde4b3b54603edd6f55aa58f64ff1bfcaa7
comment:3 Changed 4 years ago by
- Branch changed from u/adurand/derivation to u/caruso/derivation
comment:4 Changed 4 years ago by
- Commit changed from b927acde4b3b54603edd6f55aa58f64ff1bfcaa7 to a3de0749d0b734c21a6f18b7567187d37d69fd53
- Description modified (diff)
- Summary changed from Derivation over rings to Derivation over rings and Ore polynomials
New commits:
a3de074 | Initial code written by Amaury Durand
|
comment:5 Changed 4 years ago by
- Commit changed from a3de0749d0b734c21a6f18b7567187d37d69fd53 to a222da8eccf0fbd3c4cd989129b8a59b0b46d5b7
comment:6 Changed 4 years ago by
- Commit changed from a222da8eccf0fbd3c4cd989129b8a59b0b46d5b7 to 532604b87b737b35d7498440af03007fb7e7e137
Branch pushed to git repo; I updated commit sha1. New commits:
532604b | Add a parent for the module of derivations
|
comment:7 Changed 4 years ago by
- Commit changed from 532604b87b737b35d7498440af03007fb7e7e137 to cf978b3ab69c156b7a344cfec233d8884c705983
comment:8 Changed 4 years ago by
- Commit changed from cf978b3ab69c156b7a344cfec233d8884c705983 to 51f8b536cadeeaec23c364e03e380b24d865c8d2
Branch pushed to git repo; I updated commit sha1. New commits:
51f8b53 | More doctests
|
comment:9 Changed 4 years ago by
- Commit changed from 51f8b536cadeeaec23c364e03e380b24d865c8d2 to da467724e892fba5e751bcbb129aba51d4b98de5
Branch pushed to git repo; I updated commit sha1. New commits:
da46772 | Add author
|
comment:10 Changed 4 years ago by
- Description modified (diff)
- Status changed from new to needs_review
- Summary changed from Derivation over rings and Ore polynomials to Derivation and twisted derivations over rings
New commits:
da46772 | Add author
|
comment:11 Changed 4 years ago by
- Commit changed from da467724e892fba5e751bcbb129aba51d4b98de5 to ec909afd74faa63e2e7c9de85555227051e5588a
Branch pushed to git repo; I updated commit sha1. New commits:
ec909af | Add a hash function
|
comment:12 Changed 4 years ago by
- Commit changed from ec909afd74faa63e2e7c9de85555227051e5588a to 5b9bab94ce28afb5d263b2f3d37ff3286c290249
Branch pushed to git repo; I updated commit sha1. New commits:
5b9bab9 | Add doctest for is_zero
|
comment:13 follow-up: ↓ 14 Changed 4 years ago by
- Cc jsrn added
Hi Johan,
With a master student of mine (Amaury Durand), we generalized the notion of Gabidulin codes to any Ore algebra (possibly with derivation) and managed to allow more evaluation points. We are preparing a paper now and we trying to implement our generalized Gabidulin codes in Sage.
This ticket implements derivations and theta-derivations over arbitrary commutative rings. It is a first step towards implementing our generalized Gabidulin.
comment:14 in reply to: ↑ 13 ; follow-up: ↓ 17 Changed 4 years ago by
Replying to caruso:
Hi Johan,
With a master student of mine (Amaury Durand), we generalized the notion of Gabidulin codes to any Ore algebra (possibly with derivation) and managed to allow more evaluation points. We are preparing a paper now and we trying to implement our generalized Gabidulin codes in Sage.
This ticket implements derivations and theta-derivations over arbitrary commutative rings. It is a first step towards implementing our generalized Gabidulin.
Hi Xavier and Amaury,
This looks very promising. I have no experience with theta-derivations over arbitrary commutative rings, but you code seems to explain the definition nicely. I should be able to review the code in the not-too-far-future. I'd like to see a preprint of your paper (if you prefer, in private email), if some of it is ready for consumption, and if you believe it would help my reading of the code.
Best, Johan
comment:15 Changed 4 years ago by
- Commit changed from 5b9bab94ce28afb5d263b2f3d37ff3286c290249 to f4fed16b7d5fcdcefbc070da60e1e35cdc8c418f
Branch pushed to git repo; I updated commit sha1. New commits:
f4fed16 | Fix doctest
|
comment:16 follow-up: ↓ 18 Changed 4 years ago by
- Cc tscrim added
- Milestone changed from sage-8.2 to sage-8.4
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to needs_work
This is a good advancement and looks in good shape. I have a few comments and suggestions.
The set of all derivations forms a Lie algebra with [X, Y] = X*Y - Y*X
, which Sage has support for. So for untwisted derivations, it should be in the category of LieAlgebras
. Let me know if you have any questions about adding this. (The twisted derivations forms what is known as a hom-Lie algebra, where the Jacobi relation also needs to be twisted.)
Additionally, when the ring is a finite-dimensional algebra with an explicit basis, we can compute a basis for the derivation module in terms of matrices:
sage: E.<x,y> = ExteriorAlgebra(QQ) sage: E.derivations_basis() ( [0 0 0 0] [0 0 0 0] [0 0 0 0] [0 0 0 0] [0 0 0 0] [0 0 0 0] [0 1 0 0] [0 0 1 0] [0 0 0 0] [0 0 0 0] [0 0 0 0] [0 0 0 0] [0 0 0 0] [0 0 0 0] [0 1 0 0] [0 0 1 0] [0 0 0 0] [0 0 0 0] [0 0 0 1], [0 0 0 0], [0 0 0 0], [0 0 0 1], [0 1 0 0], [0 0 1 0] )
Although incorporating that is something that could easily be on a followup ticket as it will likely require a bit of extending your current implementation.
A few quick comments from reading the browsing the code:
- You should not set
self.element_class
attribute, but instead setself.Element
(theParent.__init__
ends up creating the new subclasselement_class
using stuff from the category). - Python convention is error messages do not start with a capital letter.
- Remove the first
::
here::: Twisted derivations and handled similarly::
Returns
->Return
.``n``th
->``n``-th
as otherwise it does not get parsed correctly.``n`` - an integer (default: ``0``)
->-``n`` -- integer (default: ``0``)
- It might actually be good to have
RingDerivation
still inherit from(Ring)Map
as the composition just becomes a formal composition of maps in the homspace (IIRC the default behavior). Plus it allows for natural functionality. - I think it would be better to remove
derivation_twisted
and just have an extra input toderivation
. It will have better localization of code and documentation and match the semantic ofderivation_module
.
comment:17 in reply to: ↑ 14 Changed 4 years ago by
Replying to jsrn:
This looks very promising. I have no experience with theta-derivations over arbitrary commutative rings, but you code seems to explain the definition nicely. I should be able to review the code in the not-too-far-future.
Great!
I'd like to see a preprint of your paper (if you prefer, in private email), if some of it is ready for consumption, and if you believe it would help my reading of the code.
OK, we'll send it to you... as soon as it is ready.
comment:18 in reply to: ↑ 16 ; follow-ups: ↓ 19 ↓ 20 Changed 4 years ago by
Thanks Travis for your comments.
Replying to tscrim:
The set of all derivations forms a Lie algebra with
[X, Y] = X*Y - Y*X
, which Sage has support for. So for untwisted derivations, it should be in the category ofLieAlgebras
. Let me know if you have any questions about adding this. (The twisted derivations forms what is known as a hom-Lie algebra, where the Jacobi relation also needs to be twisted.)
Good point. I'll add it. (I think I can do it by myself but I'll ask you if I have questions.)
Are the support in Sage for hom-Lie algebras?
- You should not set
self.element_class
attribute, but instead setself.Element
(theParent.__init__
ends up creating the new subclasselement_class
using stuff from the category).
Ah, OK.
- Python convention is error messages do not start with a capital letter.
Really? Is it the Sage convention as well? I ask the question because I remember that, one time, somebody told me that I should capitalize my error messages.
- It might actually be good to have
RingDerivation
still inherit from(Ring)Map
as the composition just becomes a formal composition of maps in the homspace (IIRC the default behavior). Plus it allows for natural functionality.
Actually, I already tried to inherit from RingMap
but had some troubles. But I can try harder :-)
- I think it would be better to remove
derivation_twisted
and just have an extra input toderivation
. It will have better localization of code and documentation and match the semantic ofderivation_module
.
The semantic of derivation
is a bit different from that of derivation_twisted
. It's the reason why I've implemented two distinct methods.
By the way, I just realized that my current code works almost only for polynomial algebras. In particular, it does not work for quotients of polynomial algebras: d/dx
does not define a derivation over A[x]/P(x)
but it does define a derivation from A[x]/P(x)
to A[x]/(P(x),P'(x))
. I don't know what is the best way to fix this: I can certainly move the method derivation
to the classes PolynomialRing_general
, MPolynomialRing_base
and FractionField_generic
) but this implies code duplication. Otherwise, I can leave it in the class Ring
but check if self
is an instance of those classes. I don't know if it's a good practice.
comment:19 in reply to: ↑ 18 Changed 4 years ago by
Replying to caruso:
By the way, I just realized that my current code works almost only for polynomial algebras. In particular, it does not work for quotients of polynomial algebras:
d/dx
does not define a derivation overA[x]/P(x)
but it does define a derivation fromA[x]/P(x)
toA[x]/(P(x),P'(x))
. I don't know what is the best way to fix this: I can certainly move the methodderivation
to the classesPolynomialRing_general
,MPolynomialRing_base
andFractionField_generic
) but this implies code duplication. Otherwise, I can leave it in the classRing
but check ifself
is an instance of those classes. I don't know if it's a good practice.
In fact, I think I can test it in the __init
function of RingDerivation
.
So everything's fine.
comment:20 in reply to: ↑ 18 ; follow-up: ↓ 22 Changed 4 years ago by
Replying to caruso:
Thanks Travis for your comments.
Replying to tscrim:
The set of all derivations forms a Lie algebra with
[X, Y] = X*Y - Y*X
, which Sage has support for. So for untwisted derivations, it should be in the category ofLieAlgebras
. Let me know if you have any questions about adding this. (The twisted derivations forms what is known as a hom-Lie algebra, where the Jacobi relation also needs to be twisted.)Good point. I'll add it. (I think I can do it by myself but I'll ask you if I have questions.)
Thank you.
Are there support in Sage for hom-Lie algebras?
Currently no. It is not hard to add generic support in the form of having a category and lifting the bracket
methods. However, I am not sure how much of the general theory (and hence code) could be lifted from Lie algebras to hom-Lie algebras.
- Python convention is error messages do not start with a capital letter.
Really? Is it the Sage convention as well? I ask the question because I remember that, one time, somebody told me that I should capitalize my error messages.
Yes. Whomever said otherwise was mistaken (granted there are exceptions when the message needs to be really long, but these are quite rare). At least, that is the consensus I have understood.
- It might actually be good to have
RingDerivation
still inherit from(Ring)Map
as the composition just becomes a formal composition of maps in the homspace (IIRC the default behavior). Plus it allows for natural functionality.Actually, I already tried to inherit from
RingMap
but had some troubles. But I can try harder :-)
If it was giving you difficulties, then feel free to drop it. It is not a big issue. I am happy to help try and debug things here as well.
- I think it would be better to remove
derivation_twisted
and just have an extra input toderivation
. It will have better localization of code and documentation and match the semantic ofderivation_module
.The semantic of
derivation
is a bit different from that ofderivation_twisted
. It's the reason why I've implemented two distinct methods.
Somewhat, but it feels a little artificial. You could just have a keyword only argument of twist
, which I feel is quite natural to have. However, I don't feel that strongly about it, so if you would rather keep the current constructs, then it is fine by me.
By the way, I just realized that my current code works almost only for polynomial algebras. In particular, it does not work for quotients of polynomial algebras:
d/dx
does not define a derivation overA[x]/P(x)
but it does define a derivation fromA[x]/P(x)
toA[x]/(P(x),P'(x))
. I don't know what is the best way to fix this: I can certainly move the methodderivation
to the classesPolynomialRing_general
,MPolynomialRing_base
andFractionField_generic
) but this implies code duplication. Otherwise, I can leave it in the classRing
but check ifself
is an instance of those classes. I don't know if it's a good practice.
I feel like it should work with quotients: If a map is a derivation in the ambient space, so when taking the quotient (which is functorial), you are preserving the equality. Perhaps I am over-simplifying things. I am guessing you have an explicit example in mind?
comment:21 Changed 4 years ago by
- Commit changed from f4fed16b7d5fcdcefbc070da60e1e35cdc8c418f to 568ba75735f31d338542aafa76b5e90204cf949d
comment:22 in reply to: ↑ 20 Changed 4 years ago by
Replying to tscrim:
By the way, I just realized that my current code works almost only for polynomial algebras. In particular, it does not work for quotients of polynomial algebras:
d/dx
does not define a derivation overA[x]/P(x)
but it does define a derivation fromA[x]/P(x)
toA[x]/(P(x),P'(x))
. I don't know what is the best way to fix this: I can certainly move the methodderivation
to the classesPolynomialRing_general
,MPolynomialRing_base
andFractionField_generic
) but this implies code duplication. Otherwise, I can leave it in the classRing
but check ifself
is an instance of those classes. I don't know if it's a good practice.I feel like it should work with quotients: If a map is a derivation in the ambient space, so when taking the quotient (which is functorial), you are preserving the equality. Perhaps I am over-simplifying things. I am guessing you have an explicit example in mind?
Well, if A = B (mod P)
, we can write A = B + PQ
and taking the derivative, one gets A' = B' + P'Q + PQ'
meaning that A' = B' (mod (P,P'))
but in general A' != B' (mod P)
.
For a concrete example, take P = X^n
and observe that the derivative of a polynomial defined modulo X^n
is a polynomial defined modulo X^(n-1)
(basically, we shift coefficients).
comment:23 Changed 4 years ago by
Ah, I know where I was going wrong: taking a quotient by a fixed ideal is not functorial, you need the ideal to be in the kernel.
comment:24 Changed 4 years ago by
- Commit changed from 568ba75735f31d338542aafa76b5e90204cf949d to d380337fb98e26781a6099c731ba971305009bf4
Branch pushed to git repo; I updated commit sha1. New commits:
d380337 | New version with less bugs (e.g. derivations over iterated polynomial rings now works) and more functionalities (Lie bracket, pth power, etc.)
|
comment:25 follow-up: ↓ 28 Changed 4 years ago by
- Status changed from needs_work to needs_review
I've redesigned the way derivations are handled in order to fix the bug I pointed out above (and some others) and addressed some of your remarks.
I tried again to make RingDerivation
inherit from RingMap
but it's tricky because it implies that the parent RingDerivationModule
must inherit from Homset
, which now implies to define a method homset_category
specifying in which category are the elements (i.e. the derivations). Since the derivations are not the morphims in any category (they are not stable under composition), I gave up.
Something we can implement is two methods precompose
and postcompose
that precompose/postcompose a derivation with a ring homomorphism (in which case, the result is again a derivation). The tricky point is that we get this way a derivation from a ring A
to B
where B
is a A
-algebra but the defining morphism might not be the coercion morphism (if it exists). So we should have support for this (which is good thing to have I think but needs some extra work).
comment:26 Changed 4 years ago by
- Commit changed from d380337fb98e26781a6099c731ba971305009bf4 to b588b092e5e499bfe312586c71578da7b4842306
Branch pushed to git repo; I updated commit sha1. New commits:
b588b09 | Fix doctest
|
comment:27 Changed 4 years ago by
- Commit changed from b588b092e5e499bfe312586c71578da7b4842306 to 77dce7bb150bd0cb51d74913a36f875726b8f8bb
Branch pushed to git repo; I updated commit sha1. New commits:
e037783 | Implement comparison between derivations
|
5b1cb68 | Add support for algebras defined by any ring homomorphism
|
0017f96 | Add precompose and postcompose
|
77dce7b | Check that the morphisms passed to the methods precompose/postcompose are indeed ring homomorphisms
|
comment:28 in reply to: ↑ 25 Changed 4 years ago by
Replying to caruso:
Something we can implement is two methods
precompose
andpostcompose
that precompose/postcompose a derivation with a ring homomorphism (in which case, the result is again a derivation). The tricky point is that we get this way a derivation from a ringA
toB
whereB
is aA
-algebra but the defining morphism might not be the coercion morphism (if it exists). So we should have support for this (which is good thing to have I think but needs some extra work).
OK. I've implemented this.
comment:29 follow-up: ↓ 31 Changed 4 years ago by
Thank you for adding the Lie algebra support and your additional changes and experiments. It is a little sad the Morphism
framework is not working here, but that is a limitation of the current implementation. This should be my last group of questions.
I think there is something we should be careful about in the module doc. If A
is not a commutative ring, then B
needs to be an A
-bimodule (not necessarily an algebra) to be a derivation. However, there is a left and a right action of A
on B
used in the definition. (With the code itself, this is not a problem because you enforce A
to be commutative.)
I feel like, at least for ease of maintenance, it would be better for basis
to return self._basis
even if it is _gens
. It helps keep these as separate constructs and allows us later to take a larger generator set and then reduce it to a basis.
You need to add derivation.py
to the documentation.
Replace _cmp_
with _richcmp_
as this will make it Python3 compatible.
It is better to use
-self.parent().codomain()(0) +self.parent().codomain().zero()
since the latter is cached and avoids coercion/conversion.
I think your elements need to implement a monomial_coefficients()
method, where the output is a dict
whose keys are the basis indices and the values are the corresponding coefficients.
I think (dual_)basis
should return a Family
(or a Sequence
, but this has a more limited feature set) to be consistent with other parts of Sage in ModulesWithBasis
.
Every __init__
should have a TestSuite(foo).run()
call. This is usually a good way to catch bugs and implementation requirements from the category.
I think a better __hash__
test would be
sage: hash(M) == hash((M.domain(), M.codomain(), M.twisting_morphism()) True
as it shows that the hash works and what it should be equal to.
-``n`` - an integer (default: ``0``) +-``n`` -- an integer (default: ``0``)
Instead of self(x)
, why not do self.element_class(self, x)
? Granted, it is not going to make a big difference in speed, but I think it is a little better because it is more direct.
In the same vein, for the arithmetic operations, it is faster to do, e.g.,
-return self.parent()(self._scalar - other._scalar) +return type(self)(self.parent(), self._scalar - other._scalar)
Why all the same definition of __hash__
? You are not doing a new __eq__
/__richcmp__
, so there should not be a need to do this. Why not have this be in the ABC and override when you need to?
General question, how much speed do you think you need with these elements? It might be worthwhile to consider Cythonizing the elements.
comment:30 Changed 4 years ago by
- Commit changed from 77dce7bb150bd0cb51d74913a36f875726b8f8bb to c9deb7f95efb4d87907c27b738027be838578f23
Branch pushed to git repo; I updated commit sha1. New commits:
c9deb7f | Address some of Travis' comments
|
comment:31 in reply to: ↑ 29 ; follow-up: ↓ 32 Changed 4 years ago by
Replying to tscrim:
I think there is something we should be careful about in the module doc. If
A
is not a commutative ring, thenB
needs to be anA
-bimodule (not necessarily an algebra) to be a derivation. However, there is a left and a right action ofA
onB
used in the definition. (With the code itself, this is not a problem because you enforceA
to be commutative.)
What do you propose concretely? I can add a doctest demonstrating that derivations are not supported over noncommutative rings. How does it sound?
I feel like, at least for ease of maintenance, it would be better for
basis
to returnself._basis
even if it is_gens
. It helps keep these as separate constructs and allows us later to take a larger generator set and then reduce it to a basis.
Sure!
You need to add
derivation.py
to the documentation.
I'm unsure what I'm supposed to do exactlt? It is fine now?
Replace
_cmp_
with_richcmp_
as this will make it Python3 compatible.
OK. So far, I've just implemented ==
and !=
. Should I implement the other rich comparison operators as well? (But I don't know how they should behave.)
It is better to use
-self.parent().codomain()(0) +self.parent().codomain().zero()since the latter is cached and avoids coercion/conversion.
OK.
I think your elements need to implement a
monomial_coefficients()
method, where the output is adict
whose keys are the basis indices and the values are the corresponding coefficients.
Done.
I think
(dual_)basis
should return aFamily
(or aSequence
, but this has a more limited feature set) to be consistent with other parts of Sage inModulesWithBasis
.
Well, ok for returning a Family
...
Every
__init__
should have aTestSuite(foo).run()
call. This is usually a good way to catch bugs and implementation requirements from the category.
I'll do it.
I think a better
__hash__
test would besage: hash(M) == hash((M.domain(), M.codomain(), M.twisting_morphism()) Trueas it shows that the hash works and what it should be equal to.
OK.
-``n`` - an integer (default: ``0``) +-``n`` -- an integer (default: ``0``)
Done.
Instead of
self(x)
, why not doself.element_class(self, x)
? Granted, it is not going to make a big difference in speed, but I think it is a little better because it is more direct.
I've changed this.
In the same vein, for the arithmetic operations, it is faster to do, e.g.,
-return self.parent()(self._scalar - other._scalar) +return type(self)(self.parent(), self._scalar - other._scalar)
OK. I'll change this.
Why all the same definition of
__hash__
? You are not doing a new__eq__
/__richcmp__
, so there should not be a need to do this. Why not have this be in the ABC and override when you need to?
It seems that you must have to redefine __hash__
in each class even if the code does not change (otherwise I got an error). I don't know why.
General question, how much speed do you think you need with these elements? It might be worthwhile to consider Cythonizing the elements.
For now, I'm not that concerned with speed. But on the other hand, I'm aware that my code is rather slow (especially for iteration polynomial rings). I will probably (one day) consider Cythonizing derivations but, except if you strongly disagree, it will be for another ticket.
comment:32 in reply to: ↑ 31 Changed 4 years ago by
Replying to caruso:
Replying to tscrim:
I think there is something we should be careful about in the module doc. If
A
is not a commutative ring, thenB
needs to be anA
-bimodule (not necessarily an algebra) to be a derivation. However, there is a left and a right action ofA
onB
used in the definition. (With the code itself, this is not a problem because you enforceA
to be commutative.)What do you propose concretely? I can add a doctest demonstrating that derivations are not supported over noncommutative rings. How does it sound?
I don't think it needs a doctest since for the actual implementation you say commutative ring. I think it is sufficient to just replace "algebra over A
" with "A
-bimodule" in the top-level docstring.
It might be worthwhile mentioning somewhere (maybe RingDerivationModule
, but maybe elsewhere) that when B = A
, then the set of (twisted) derivations has a (hom-)Lie algebra structure. However, I am bias because I like Lie algebras. :)
You need to add
derivation.py
to the documentation.I'm unsure what I'm supposed to do exactlt? It is fine now?
Yes, that is what was missing.
Replace
_cmp_
with_richcmp_
as this will make it Python3 compatible.OK. So far, I've just implemented
==
and!=
. Should I implement the other rich comparison operators as well? (But I don't know how they should behave.)
No, you shouldn't. In fact, that is once of the nice things about _richcmp_
is you do not need other comparisons. You can just return a NotImplemented
. Although, you could just do
from sage.structure.richcmp import richcmp # put this import at the top level return richcmp(self.list(), other.list(), op)
I think
(dual_)basis
should return aFamily
(or aSequence
, but this has a more limited feature set) to be consistent with other parts of Sage inModulesWithBasis
.Well, ok for returning a
Family
...
Thank you. This also makes it so self.basis().cardinality()
and self.basis().keys()
works and helps make things uniform.
Why all the same definition of
__hash__
? You are not doing a new__eq__
/__richcmp__
, so there should not be a need to do this. Why not have this be in the ABC and override when you need to?It seems that you must have to redefine
__hash__
in each class even if the code does not change (otherwise I got an error). I don't know why.
That is very strange. I would figure it would be sufficient for some ABC. I might have to look more closely into that.
General question, how much speed do you think you need with these elements? It might be worthwhile to consider Cythonizing the elements.
For now, I'm not that concerned with speed. But on the other hand, I'm aware that my code is rather slow (especially for iteration polynomial rings). I will probably (one day) consider Cythonizing derivations but, except if you strongly disagree, it will be for another ticket.
Another ticket is fine. It is slightly easier to split the element class into a Cython file here than on a separate ticket, but only slightly. I am fine with whatever you decide.
comment:33 Changed 4 years ago by
- Commit changed from c9deb7f95efb4d87907c27b738027be838578f23 to 369db6af5d5d939f10f7b567715753a86ba87963
Branch pushed to git repo; I updated commit sha1. New commits:
369db6a | Address Travis' review
|
comment:34 Changed 4 years ago by
There is still a bug with pickling but it's not because of this ticket.
I've opened up a new ticket (#26354) reporting the bug.
comment:35 Changed 4 years ago by
- Dependencies set to #26354
comment:36 Changed 4 years ago by
- Commit changed from 369db6af5d5d939f10f7b567715753a86ba87963 to 267f74db4e8a9dafa32dba059a74302a5b2c1522
comment:37 Changed 4 years ago by
- Branch changed from u/caruso/derivation to u/tscrim/polynomial_derivations-25134
- Commit changed from 267f74db4e8a9dafa32dba059a74302a5b2c1522 to f0878ebfc46ce776f011ec4d9b059be8bb05215c
Thank you for all the changes. I made a few reviewer changes to fix the docbuild error, some broken doc links, bad formatting, and other small adjustments. I also did a bunch of PEP8 consistency changes.
If my changes are good with you, then positive review.
New commits:
f0878eb | Fixing some docstrings and some PEP8.
|
comment:38 Changed 4 years ago by
- Branch changed from u/tscrim/polynomial_derivations-25134 to u/caruso/polynomial_derivations-25134
comment:39 Changed 4 years ago by
I've fixed a few typos in the doctest.
I can give a positive review now but maybe it's better to wait for #26354. What's your opinion?
comment:40 Changed 4 years ago by
- Commit changed from f0878ebfc46ce776f011ec4d9b059be8bb05215c to dbe85c5ad7d56bf63c9d26b48669f02aa6b71ce7
Branch pushed to git repo; I updated commit sha1. New commits:
dbe85c5 | Typos
|
comment:41 Changed 4 years ago by
- Status changed from needs_review to positive_review
It's a positive review, but it won't get merged until the dependency is merged.
comment:42 Changed 4 years ago by
- Commit changed from dbe85c5ad7d56bf63c9d26b48669f02aa6b71ce7 to c8fee99e5ac2c4148b885cb33367d4b64da7822c
- Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
dee878e | Revert "Set immutable in unpickling ring morphisms"
|
e2931b6 | Fix pickling for PolynomialSequence_generic
|
1ea91f7 | Add a doctest
|
c8fee99 | Merge branch 't/26354/pickling_morphisms_is_broken' into t/25134/polynomial_derivations-25134
|
comment:43 Changed 4 years ago by
- Status changed from needs_review to positive_review
I just merged with #26354. Actually, I'm not sure it was needed but it's probably safer like this.
comment:44 Changed 4 years ago by
- Branch changed from u/caruso/polynomial_derivations-25134 to c8fee99e5ac2c4148b885cb33367d4b64da7822c
- Resolution set to fixed
- Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
add method to create derivation in polynomial ring