Opened 4 years ago
Closed 3 years ago
#27442 closed enhancement (fixed)
Implement option for factoring differentials out of Weyl algebras
Reported by:  Travis Scrimshaw  Owned by:  

Priority:  major  Milestone:  sage8.8 
Component:  algebra  Keywords:  Weyl algebra 
Cc:  Frédéric Chapoton  Merged in:  
Authors:  Travis Scrimshaw  Reviewers:  Daniel Krenn 
Report Upstream:  N/A  Work issues:  
Branch:  cd2a29e (Commits, GitHub, GitLab)  Commit:  cd2a29e0ac1a27c71914484d8ed1bbf30afa8582 
Dependencies:  Stopgaps: 
Description (last modified by )
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
Change History (14)
comment:1 Changed 4 years ago by
Branch:  → public/algebras/display_options_weyl27442 

Commit:  → 2c071483f74b464bf96849be811814fdffa27f4d 
Description:  modified (diff) 
Status:  new → needs_review 
comment:2 Changed 4 years ago by
Cc:  Frédéric Chapoton added 

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
Milestone:  sage8.7 → sage8.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 followup: 8 Changed 4 years ago by
A couple of small remarks:
for e,g in
: Not sure if PEP8 would sayfor e, g in
(space after comma) here. (found three times)[True,False]
(very last line of patch): here PEP8 for sure Also
R.<x,y,z>
,x,y,z,dx,dy,dz
,t,dt
should IMO been written with space after comma. factor_differentials
is not doctested.(8*t^3 + 18*t)dt^1
(discussion): Should there be a*
before thedt
? (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). 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
Reviewers:  → Daniel Krenn 

Status:  needs_review → needs_work 
comment:6 Changed 3 years ago by
Commit:  2c071483f74b464bf96849be811814fdffa27f4d → f3e3321289aa880d76c0ed688d5eec03cad7de3a 

comment:7 Changed 3 years ago by
Commit:  f3e3321289aa880d76c0ed688d5eec03cad7de3a → 2b0e2be9f1db457169bddcfe3b63491cabe8f819 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
2b0e2be  Changes from reviewer comments.

comment:8 followup: 9 Changed 3 years ago by
Replying to dkrenn:
A couple of small remarks:
for e,g in
: Not sure if PEP8 would sayfor 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.
[True,False]
(very last line of patch): here PEP8 for sure
See above.
 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.
factor_differentials
is not doctested.
Whoops, thanks. Fixed.
(8*t^3 + 18*t)dt^1
(discussion): Should there be a*
before thedt
? (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.
 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 d_{t} 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 Changed 3 years ago by
Status:  needs_work → needs_review 

Replying to tscrim:
Replying to dkrenn:
A couple of small remarks:
for e,g in
: Not sure if PEP8 would sayfor 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.
 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.
factor_differentials
is not doctested.Whoops, thanks. Fixed.
LGTM.
(8*t^3 + 18*t)dt^1
(discussion): Should there be a*
before thedt
? (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.
 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 d_{t} 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
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
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 followup: 13 Changed 3 years ago by
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.
comment:13 Changed 3 years ago by
Replying to tscrim:
I fixed that one doctest and [...] 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.
Fine for me, thank you.
Thank you for the review.
You're welcome.
comment:14 Changed 3 years ago by
Branch:  public/algebras/display_options_weyl27442 → cd2a29e0ac1a27c71914484d8ed1bbf30afa8582 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
Adding factored representations to Weyl algebra elements.