#27442 closed enhancement (fixed)

Implement option for factoring differentials out of Weyl algebras

Reported by: tscrim Owned by:
Priority: major Milestone: sage-8.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) Commit: cd2a29e0ac1a27c71914484d8ed1bbf30afa8582
Dependencies: Stopgaps:

Description (last modified by tscrim)

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

  • Branch set to public/algebras/display_options_weyl-27442
  • Commit set to 2c071483f74b464bf96849be811814fdffa27f4d
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

2c07148Adding factored representations to Weyl algebra elements.

comment:2 Changed 11 months ago by tscrim

  • 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 10 months ago by embray

  • Milestone changed from sage-8.7 to 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: Changed 10 months ago by 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)
  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 10 months ago by dkrenn

  • Reviewers set to Daniel Krenn
  • Status changed from needs_review to needs_work

comment:6 Changed 10 months ago by git

  • Commit changed from 2c071483f74b464bf96849be811814fdffa27f4d to f3e3321289aa880d76c0ed688d5eec03cad7de3a

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

  • Commit changed from f3e3321289aa880d76c0ed688d5eec03cad7de3a to 2b0e2be9f1db457169bddcfe3b63491cabe8f819

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 ; follow-up: Changed 10 months ago by 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.

  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 10 months ago by dkrenn

  • Status changed from needs_work to needs_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 10 months ago by tscrim

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

  • Commit changed from 2b0e2be9f1db457169bddcfe3b63491cabe8f819 to cd2a29e0ac1a27c71914484d8ed1bbf30afa8582

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

cd2a29eChanges from reviewer comments.

comment:12 follow-up: Changed 10 months ago by tscrim

  • 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 10 months ago by dkrenn

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

  • Branch changed from public/algebras/display_options_weyl-27442 to cd2a29e0ac1a27c71914484d8ed1bbf30afa8582
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.