Opened 4 years ago

Closed 3 years ago

# Implement option for factoring differentials out of Weyl algebras

Reported by: Owned by: Travis Scrimshaw major sage-8.8 algebra Weyl algebra Frédéric Chapoton Travis Scrimshaw Daniel Krenn N/A cd2a29e cd2a29e0ac1a27c71914484d8ed1bbf30afa8582

It is natural to write the elements of the (differential) Weyl algebra with the differentials factored out (on the right). This is a common expression for the elements in the single variable case.

With this branch

sage: R.<t> = QQ[]
sage: D = DifferentialWeylAlgebra(R)
sage: t,dt = D.gens()
sage: x = dt^3*t^3 + dt^2*t^4
sage: x
t^3*dt^3 + t^4*dt^2 + 9*t^2*dt^2 + 8*t^3*dt + 18*t*dt + 12*t^2 + 6
sage: D.options.factor_representation = True
sage: x
(12*t^2 + 6) + (8*t^3 + 18*t)dt^1 + (t^4 + 9*t^2)dt^2 + (t^3)dt^3


### comment:1 Changed 4 years ago by Travis Scrimshaw

Branch: → public/algebras/display_options_weyl-27442 → 2c071483f74b464bf96849be811814fdffa27f4d modified (diff) new → needs_review

New commits:

 ​2c07148 Adding factored representations to Weyl algebra elements.

### comment:2 Changed 4 years ago by Travis Scrimshaw

I would appreciate a review here (bot is morally green), but I will understand if you do not want to as this is not a trivial ticket.

### comment:3 Changed 4 years ago by Erik Bray

Milestone: sage-8.7 → sage-8.8

Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)

### comment:4 follow-up:  8 Changed 4 years ago by Daniel Krenn

A couple of small remarks:

1. for e,g in: Not sure if PEP8 would say for e, g in (space after comma) here. (found three times)
2. [True,False] (very last line of patch): here PEP8 for sure
3. Also R.<x,y,z>, x,y,z,dx,dy,dz, t,dt should IMO been written with space after comma.
4. factor_differentials is not doctested.
5. (8*t^3 + 18*t)dt^1 (discussion): Should there be a * before the dt? (In some sense, this would be closer to a representation that one could feed back into the system and let it evaluate (i.e. correct Python syntax). However, I am aware that this might not be a major usecase (if at all).
6. Latex d^{3}_{t}: I simply do not know if this is the standard convention to write to typeset it; I simply believe you here and just wanted it noted.

Otherwise, LGTM.

### comment:5 Changed 4 years ago by Daniel Krenn

Reviewers: → Daniel Krenn needs_review → needs_work

### comment:6 Changed 3 years ago by git

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

 ​dc126b7 Merge branch 'public/algebras/display_options_weyl-27442' of git://trac.sagemath.org/sage into public/algebras/display_options_weyl-27442 ​f3e3321 Changes from reviewer comments.

### comment:7 Changed 3 years ago by git

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

 ​2b0e2be Changes from reviewer comments.

### comment:8 in reply to:  4 ; follow-up:  9 Changed 3 years ago by Travis Scrimshaw

A couple of small remarks:

1. for e,g in: Not sure if PEP8 would say for e, g in (space after comma) here. (found three times)

PEP8 allows you to remove the space for operators when inside of a lower precedence operator. So IMO the no space is fine (and can be more readable). However, I don't care so strongly and have changed it.

1. [True,False] (very last line of patch): here PEP8 for sure

See above.

1. Also R.<x,y,z>, x,y,z,dx,dy,dz, t,dt should IMO been written with space after comma.

IMO, the R.<x,y,z> looks better and is easier to read (and fits PEP8 in the sense given above). However, I agree that there can be spacing for the list of generators, so I have changed those.

1. factor_differentials is not doctested.

Whoops, thanks. Fixed.

1. (8*t^3 + 18*t)dt^1 (discussion): Should there be a * before the dt? (In some sense, this would be closer to a representation that one could feed back into the system and let it evaluate (i.e. correct Python syntax). However, I am aware that this might not be a major usecase (if at all).

That is a good point. I have added that.

1. Latex d^{3}_{t}: I simply do not know if this is the standard convention to write to typeset it; I simply believe you here and just wanted it noted.

It was a good thing to note. In PDEs, people use dt for the derivative wrt t. However, in this case, I used the del/\partial notation, so I changed it to be consistent with that.

### comment:9 in reply to:  8 Changed 3 years ago by Daniel Krenn

Status: needs_work → needs_review

A couple of small remarks:

1. for e,g in: Not sure if PEP8 would say for e, g in (space after comma) here. (found three times)

PEP8 allows you to remove the space for operators when inside of a lower precedence operator. So IMO the no space is fine (and can be more readable). However, I don't care so strongly and have changed it.

I see. I know of this rule, but never interpreted it for skipping space after a comma. If I would have known that, I might would have reviewed differently. Anyways, thanks for changing; I think that in many parts of SageMath, there are spaces after commas.

1. Also R.<x,y,z>, x,y,z,dx,dy,dz, t,dt should IMO been written with space after comma.

IMO, the R.<x,y,z> looks better and is easier to read (and fits PEP8 in the sense given above). However, I agree that there can be spacing for the list of generators, so I have changed those.

Thanks. LGTM.

1. factor_differentials is not doctested.

Whoops, thanks. Fixed.

LGTM.

1. (8*t^3 + 18*t)dt^1 (discussion): Should there be a * before the dt? (In some sense, this would be closer to a representation that one could feed back into the system and let it evaluate (i.e. correct Python syntax). However, I am aware that this might not be a major usecase (if at all).

That is a good point. I have added that.

Thanks.

1. Latex d^{3}_{t}: I simply do not know if this is the standard convention to write to typeset it; I simply believe you here and just wanted it noted.

It was a good thing to note. In PDEs, people use dt for the derivative wrt t. However, in this case, I used the del/\partial notation, so I changed it to be consistent with that.

Ok, thank you for the explanation.

So, for me this everything looks fine. Once the patchbot is happy, this is a positive review.

### comment:10 Changed 3 years ago by Travis Scrimshaw

Thanks. PEP8 allows for some flexibility and interpretation. Bottom line is being consistent and what looks "good".

So the patchbot is essentially happy modulo one bad doctest (I swore I tested the file before pushing...), which I will fix when I get to my desktop tomorrow morning (I am based in Australia).

### comment:11 Changed 3 years ago by git

Commit: 2b0e2be9f1db457169bddcfe3b63491cabe8f819 → cd2a29e0ac1a27c71914484d8ed1bbf30afa8582

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

 ​cd2a29e Changes from reviewer comments.

### comment:12 follow-up:  13 Changed 3 years ago by Travis Scrimshaw

Status: needs_review → positive_review

I fixed that one doctest and did a force push since it was a trivial change. With that, I now get the file passing all tests:

Using --optional=4ti2,coxeter3,dochtml,dot2tex,gambit,latte_int,lidia,lrslib,meataxe,memlimit,mpir,normaliz,p_group_cohomology,pynormaliz,python2,sage,sirocco
Doctesting 1 file using 8 threads.
sage -t --long weyl_algebra.py
[196 tests, 0.16 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 0.2 seconds
cpu time: 0.2 seconds
cumulative wall time: 0.2 seconds


This with the previous green bot, I am allowing myself to set a positive review. If you disagree Daniel (and want to wait for another patchbot), just set it back to needs review.

Thank you for the review.