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: sage-8.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:

Status badges

Description (last modified by Travis Scrimshaw)

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 Travis Scrimshaw

Branch: public/algebras/display_options_weyl-27442
Commit: 2c071483f74b464bf96849be811814fdffa27f4d
Description: modified (diff)
Status: newneeds_review

New commits:

2c07148Adding factored representations to Weyl algebra elements.

comment:2 Changed 4 years ago by Travis Scrimshaw

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 Erik Bray

Milestone: sage-8.7sage-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 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
Status: needs_reviewneeds_work

comment:6 Changed 3 years ago by git

Commit: 2c071483f74b464bf96849be811814fdffa27f4df3e3321289aa880d76c0ed688d5eec03cad7de3a

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

dc126b7Merge branch 'public/algebras/display_options_weyl-27442' of git://trac.sagemath.org/sage into public/algebras/display_options_weyl-27442
f3e3321Changes from reviewer comments.

comment:7 Changed 3 years ago by git

Commit: f3e3321289aa880d76c0ed688d5eec03cad7de3a2b0e2be9f1db457169bddcfe3b63491cabe8f819

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

2b0e2beChanges from reviewer comments.

comment:8 in reply to:  4 ; Changed 3 years ago by Travis Scrimshaw

Replying to dkrenn:

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_workneeds_review

Replying to tscrim:

Replying to dkrenn:

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: 2b0e2be9f1db457169bddcfe3b63491cabe8f819cd2a29e0ac1a27c71914484d8ed1bbf30afa8582

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

cd2a29eChanges from reviewer comments.

comment:12 Changed 3 years ago by Travis Scrimshaw

Status: needs_reviewpositive_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 Daniel Krenn

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 Volker Braun

Branch: public/algebras/display_options_weyl-27442cd2a29e0ac1a27c71914484d8ed1bbf30afa8582
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.