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

Priority:  major  Milestone:  sage8.8 
Component:  algebra  Keywords:  Weyl algebra 
Cc:  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 3 years ago by
 Branch set to public/algebras/display_options_weyl27442
 Commit set to 2c071483f74b464bf96849be811814fdffa27f4d
 Description modified (diff)
 Status changed from new to needs_review
comment:2 Changed 3 years ago by
 Cc 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 3 years ago by
 Milestone changed from sage8.7 to 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 3 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 3 years ago by
 Reviewers set to Daniel Krenn
 Status changed from needs_review to needs_work
comment:6 Changed 3 years ago by
 Commit changed from 2c071483f74b464bf96849be811814fdffa27f4d to f3e3321289aa880d76c0ed688d5eec03cad7de3a
comment:7 Changed 3 years ago by
 Commit changed from f3e3321289aa880d76c0ed688d5eec03cad7de3a to 2b0e2be9f1db457169bddcfe3b63491cabe8f819
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 ; 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 in reply to: ↑ 8 Changed 3 years ago by
 Status changed from needs_work to 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 changed from 2b0e2be9f1db457169bddcfe3b63491cabe8f819 to 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 changed from needs_review to 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 in reply to: ↑ 12 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 changed from public/algebras/display_options_weyl27442 to cd2a29e0ac1a27c71914484d8ed1bbf30afa8582
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Adding factored representations to Weyl algebra elements.