Opened 8 months ago

Closed 2 months 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) Commit: c8fee99e5ac2c4148b885cb33367d4b64da7822c
Dependencies: #26354 Stopgaps:

Description (last modified by caruso)

This ticket implements derivation and theta-derivations over rings. (It would be useful for implementing general Ore polynomials.)

Change History (44)

comment:1 Changed 7 months ago by adurand

  • Branch set to u/adurand/derivation

comment:2 Changed 7 months ago by git

  • Commit set to b927acde4b3b54603edd6f55aa58f64ff1bfcaa7

Branch pushed to git repo; I updated commit sha1. New commits:

b927acdadd method to create derivation in polynomial ring

comment:3 Changed 4 months ago by caruso

  • Branch changed from u/adurand/derivation to u/caruso/derivation

comment:4 Changed 4 months ago by caruso

  • Commit changed from b927acde4b3b54603edd6f55aa58f64ff1bfcaa7 to a3de0749d0b734c21a6f18b7567187d37d69fd53
  • Description modified (diff)
  • Summary changed from Derivation over rings to Derivation over rings and Ore polynomials

New commits:

a3de074Initial code written by Amaury Durand

comment:5 Changed 3 months ago by git

  • Commit changed from a3de0749d0b734c21a6f18b7567187d37d69fd53 to a222da8eccf0fbd3c4cd989129b8a59b0b46d5b7

Branch pushed to git repo; I updated commit sha1. New commits:

823594aMerge branch 'develop' into t/25134/derivation
a222da8Implement left Euclidean division

comment:6 Changed 3 months ago by git

  • Commit changed from a222da8eccf0fbd3c4cd989129b8a59b0b46d5b7 to 532604b87b737b35d7498440af03007fb7e7e137

Branch pushed to git repo; I updated commit sha1. New commits:

532604bAdd a parent for the module of derivations

comment:7 Changed 3 months ago by git

  • Commit changed from 532604b87b737b35d7498440af03007fb7e7e137 to cf978b3ab69c156b7a344cfec233d8884c705983

Branch pushed to git repo; I updated commit sha1. New commits:

44f0ca2Implement twisted derivations
cf978b3Some documentation

comment:8 Changed 3 months ago by git

  • Commit changed from cf978b3ab69c156b7a344cfec233d8884c705983 to 51f8b536cadeeaec23c364e03e380b24d865c8d2

Branch pushed to git repo; I updated commit sha1. New commits:

51f8b53More doctests

comment:9 Changed 3 months ago by git

  • Commit changed from 51f8b536cadeeaec23c364e03e380b24d865c8d2 to da467724e892fba5e751bcbb129aba51d4b98de5

Branch pushed to git repo; I updated commit sha1. New commits:

da46772Add author

comment:10 Changed 3 months ago by caruso

  • Authors changed from Amaury Durand to Amaury Durand, Xavier Caruso
  • 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:

da46772Add author

comment:11 Changed 3 months ago by git

  • Commit changed from da467724e892fba5e751bcbb129aba51d4b98de5 to ec909afd74faa63e2e7c9de85555227051e5588a

Branch pushed to git repo; I updated commit sha1. New commits:

ec909afAdd a hash function

comment:12 Changed 3 months ago by git

  • Commit changed from ec909afd74faa63e2e7c9de85555227051e5588a to 5b9bab94ce28afb5d263b2f3d37ff3286c290249

Branch pushed to git repo; I updated commit sha1. New commits:

5b9bab9Add doctest for is_zero

comment:13 follow-up: Changed 3 months ago by caruso

  • 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: Changed 3 months ago by jsrn

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 3 months ago by git

  • Commit changed from 5b9bab94ce28afb5d263b2f3d37ff3286c290249 to f4fed16b7d5fcdcefbc070da60e1e35cdc8c418f

Branch pushed to git repo; I updated commit sha1. New commits:

f4fed16Fix doctest

comment:16 follow-up: Changed 3 months ago by tscrim

  • 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 set self.Element (the Parent.__init__ ends up creating the new subclass element_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 to derivation. It will have better localization of code and documentation and match the semantic of derivation_module.

comment:17 in reply to: ↑ 14 Changed 3 months ago by caruso

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: Changed 3 months ago by 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 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.)

Good point. I'll add it. (I think I can do it by myself but I'll ask you if I have questions.)

Are there support in Sage for hom-Lie algebras?

  • You should not set self.element_class attribute, but instead set self.Element (the Parent.__init__ ends up creating the new subclass element_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 to derivation. It will have better localization of code and documentation and match the semantic of derivation_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.

Last edited 3 months ago by caruso (previous) (diff)

comment:19 in reply to: ↑ 18 Changed 3 months ago by caruso

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 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.

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: Changed 3 months ago by tscrim

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 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.)

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 to derivation. It will have better localization of code and documentation and match the semantic of derivation_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.

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 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.

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 3 months ago by git

  • Commit changed from f4fed16b7d5fcdcefbc070da60e1e35cdc8c418f to 568ba75735f31d338542aafa76b5e90204cf949d

Branch pushed to git repo; I updated commit sha1. New commits:

2bb4bbdSmall fixes
568ba75Merge branch 'develop' into t/25134/derivation

comment:22 in reply to: ↑ 20 Changed 3 months ago by caruso

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 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.

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 3 months ago by tscrim

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 3 months ago by git

  • Commit changed from 568ba75735f31d338542aafa76b5e90204cf949d to d380337fb98e26781a6099c731ba971305009bf4

Branch pushed to git repo; I updated commit sha1. New commits:

d380337New 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: Changed 3 months ago by caruso

  • 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 3 months ago by git

  • Commit changed from d380337fb98e26781a6099c731ba971305009bf4 to b588b092e5e499bfe312586c71578da7b4842306

Branch pushed to git repo; I updated commit sha1. New commits:

b588b09Fix doctest

comment:27 Changed 3 months ago by git

  • Commit changed from b588b092e5e499bfe312586c71578da7b4842306 to 77dce7bb150bd0cb51d74913a36f875726b8f8bb

Branch pushed to git repo; I updated commit sha1. New commits:

e037783Implement comparison between derivations
5b1cb68Add support for algebras defined by any ring homomorphism
0017f96Add precompose and postcompose
77dce7bCheck that the morphisms passed to the methods precompose/postcompose are indeed ring homomorphisms

comment:28 in reply to: ↑ 25 Changed 3 months ago by caruso

Replying to caruso:

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).

OK. I've implemented this.

comment:29 follow-up: Changed 3 months ago by tscrim

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 3 months ago by git

  • Commit changed from 77dce7bb150bd0cb51d74913a36f875726b8f8bb to c9deb7f95efb4d87907c27b738027be838578f23

Branch pushed to git repo; I updated commit sha1. New commits:

c9deb7fAddress some of Travis' comments

comment:31 in reply to: ↑ 29 ; follow-up: Changed 3 months ago by 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, 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.)

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 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.

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 a dict whose keys are the basis indices and the values are the corresponding coefficients.

Done.

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.

Well, ok for returning a Family...

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'll do it.

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.

OK.

-``n`` - an integer (default: ``0``)
+-``n`` -- an integer (default: ``0``)

Done.

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.

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 3 months ago by tscrim

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, 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.)

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 a Family (or a Sequence, but this has a more limited feature set) to be consistent with other parts of Sage in ModulesWithBasis.

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 3 months ago by git

  • Commit changed from c9deb7f95efb4d87907c27b738027be838578f23 to 369db6af5d5d939f10f7b567715753a86ba87963

Branch pushed to git repo; I updated commit sha1. New commits:

369db6aAddress Travis' review

comment:34 Changed 3 months ago by caruso

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.

Last edited 3 months ago by caruso (previous) (diff)

comment:35 Changed 3 months ago by caruso

  • Dependencies set to #26354

comment:36 Changed 3 months ago by git

  • Commit changed from 369db6af5d5d939f10f7b567715753a86ba87963 to 267f74db4e8a9dafa32dba059a74302a5b2c1522

Branch pushed to git repo; I updated commit sha1. New commits:

94f6985Set immutable in unpickling ring morphisms
c4fffe2Merge branch 't/26354/pickling_morphisms_is_broken' into t/25134/derivation
267f74dImplement _richcmp_ for twisted derivations

comment:37 Changed 3 months ago by tscrim

  • 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:

f0878ebFixing some docstrings and some PEP8.

comment:38 Changed 3 months ago by caruso

  • Branch changed from u/tscrim/polynomial_derivations-25134 to u/caruso/polynomial_derivations-25134

comment:39 Changed 3 months ago by caruso

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 3 months ago by git

  • Commit changed from f0878ebfc46ce776f011ec4d9b059be8bb05215c to dbe85c5ad7d56bf63c9d26b48669f02aa6b71ce7

Branch pushed to git repo; I updated commit sha1. New commits:

dbe85c5Typos

comment:41 Changed 3 months ago by tscrim

  • 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 2 months ago by git

  • 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:

dee878eRevert "Set immutable in unpickling ring morphisms"
e2931b6Fix pickling for PolynomialSequence_generic
1ea91f7Add a doctest
c8fee99Merge branch 't/26354/pickling_morphisms_is_broken' into t/25134/polynomial_derivations-25134

comment:43 Changed 2 months ago by caruso

  • 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 2 months ago by vbraun

  • Branch changed from u/caruso/polynomial_derivations-25134 to c8fee99e5ac2c4148b885cb33367d4b64da7822c
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.