Promote

[pzinn]

This PR addresses some of the issues raised in #3184. before:

i1 : R=QQ[x]

o1 = R

o1 : PolynomialRing

i2 : S=R[y]

o2 = S

o2 : PolynomialRing

i3 : 1/x+1/y
stdio:3:3:(3): error: expected pair to have a method for '+'

i4 : matrix{{1/x}}*matrix{{1/y}}
stdio:4:13:(3): error: maps over incompatible rings

i5 : promote(1/x_R,frac S)
stdio:5:7:(3): error: no method found for applying promote to:
                   1
     argument 1 :  - (of class frac R)
                   x
     argument 2 :  frac S

i6 : lift(1/x_S,frac R)
stdio:6:4:(3): error: no method found for applying lift to:
                   1
     argument 1 :  - (of class frac S)
                   x
     argument 2 :  frac R

and

i1 : R=QQ[x]

o1 = R

o1 : PolynomialRing

i2 : T=R**QQ[z]

o2 = T

o2 : PolynomialRing

i3 : x_R*z
stdio:3:3:(3): error: expected pair to have a method for '*'

after:

i1 : R=QQ[x]

o1 = R

o1 : PolynomialRing

i2 : S=R[y]

o2 = S

o2 : PolynomialRing

i3 : 1/x+1/y

     y + x
o3 = -----
      x*y

o3 : frac S

i4 : matrix{{1/x}}*matrix{{1/y}}

o4 = | 1/xy |

                    1             1
o4 : Matrix (frac S)  <-- (frac S)

i5 : promote(1/x_R,frac S)

     1
o5 = -
     x

o5 : frac S

i6 : lift(1/x_S,frac R)

     1
o6 = -
     x

o6 : frac R

and

i1 : R=QQ[x]

o1 = R

o1 : PolynomialRing

i2 : T=R**QQ[z]

o2 = T

o2 : PolynomialRing

i3 : x_R*z

o3 = x*z

o3 : T

In particular it introduces a new method setupPromote which makes it easy to (automatically) promote from A -> B given a map f: A -> B.

[pzinn]

there is isLiftable in Varieties.m2. should it be merged with the core isLiftable?

[mahrud]

there is isLiftable in Varieties.m2. should it be merged with the core isLiftable?

Sure, you just need to remove isLiftable = method() right? That should be fine.

[mahrud]

A similar approach to your promoteFromMap that I thought about a while ago but never tried implementing is based on allowing users to install a natural ring map between any two given rings and having internal methods simply call map(R, S) to access it. e.g. I could define two completely different rings where I have a mathematically natural map between them, but defining a ring map and passing it to every function is really cumbersome, but if I could literally type:

map(R, S) := map(R, S, { ... }, DegreeLift => ..., DegreeMap => ...)

and have it cache that ring map (there is a syntax for allowing this), then promote and lift could also use it. Or perhaps we could allow something like:

lift(R, S) := ...
promote(S, R) := ...

to define these methods in one place and have it be used throughout.

The main motivation for this was having a better way to cache a ring map that specifically has a degree map or degree lift map that is then used here: https://github.com/Macaulay2/M2/blob/1fd238210e9a37ddaec3b700ccd22f06ff5f28f5/M2/Macaulay2/m2/basis.m2#L200-L203 In many situations map(R, S) is defined, but the degree map that M2 uses by default is incorrect, which causes all sorts of errors here.

[pzinn]

I'm not sure if this is what you want but map(R,S) will work after calling promoteFromMap:

i1 : R=QQ[x]

o1 = R

o1 : PolynomialRing

i2 : S=QQ[y]

o2 = S

o2 : PolynomialRing

i3 : promoteFromMap map(R,S,{x^2})

i4 : map(R,S)

                  2
o4 = map (R, S, {x })

o4 : RingMap R <-- S

[pzinn]

there is isLiftable in Varieties.m2. should it be merged with the core isLiftable?

Sure, you just need to remove isLiftable = method() right? That should be fine.

That seems to cause problems. I haven't looked in detail but the Core isLiftable is a method(TypicalValue => Boolean, Dispatch => {Thing, Type, Type}) whereas the one in Varieties isn't, so the isLiftable(Matrix, Matrix) there probably doesn't work.

[mahrud]

Okay, then either change the method type or don't rename it?

[pzinn]

not sure what you mean by either option. Do you want to change isLiftable(Matrix, Matrix) to take different arguments?

[mahrud]

No, change the isLiftable in Core. For instance you could use hooks to check lift(r, R) =!= null by default:

isLiftable(Number,      Ring) :=
isLiftable(RingElement, Ring) := (r, R) -> ring r === R or tryHooks(
    LiftabilityHooks, (r, R), (r, R) -> lift(r, R) =!= null))

Then you only need to install the special cases where there's legitimately a way to check liftability without actually lifting like this:

addHook(LiftabilityHooks, (r, R) -> if ring r === QQ and R === ZZ then denominator r === 1)

Actually, I would love to use hooks like this for lift and promote and stop this mess:

i49 : #methods lift

o49 = 103

i50 : #methods promote

o50 = 124

[pzinn]

I have to disagree with you here. I don't think hooks should be used as a replacement for the traditional method paradigm based on inheritance. And changing the definition of the Core isLiftable to not be Dispatch => {Thing, Type, Type} would be a major change which would need to be discussed with all involved developers. In any case, definitely outside the scope of this PR.

[mahrud]

Okay, then don't rename liftable.

[pzinn]

OK I can revert that part of the PR. Though at some point we'll want to do the rename, and then the problem will arise again.

[mahrud]

Well, I provided a way for that to happen, but you disagreed, and I don't see another option.

I don't think hooks should be used as a replacement for the traditional method paradigm based on inheritance.

The problem with lift, promote and liftable is specifically that they do not use inheritance because e.g. QQ[x] is not a subtype of QQ, so why are they implemented as methods in the first place? The whole Dispatch => Thing system seems to be kind of a hack, given that rings and modules don't have inheritance whatsoever.

Besides, we use hooks in places where a function depends on the specifics of the ring of its inputs several places; look up ReduceHooks and ContainmentHooks for instance. This seems like the natural application to me.

[pzinn]

I don't disagree that Dispatch is a bit of a hack. If M2internals weren't at 3am for me, I'd be happy to suggest a general discussion about it at some point.

[pzinn]

reverting liftable -> isLiftable 7689295449e5036a534d0726da5408f378bb862c e9e77da2693fedcadde05cccfcc957e0f444e745 a7e9349fe533bc49bda54387554786a5730c5e70

[mahrud]

Are you aiming for this PR to make it to the upcoming release, or the next one?

[pzinn]

I think it should be ready for this release.

[pzinn]

seems I'm getting both #3239 and #1579 ?

[pzinn]

Looks good to me.

Tasks for future:

• rename liftable to isLiftable
• allow the notation map(R, S) = map(R, S, {x,y,z}, DegreeMap => ..., DegreeLift => ...)
• perhaps limit ring-dependent lift/promote methods to only ring elements

Yes this PR is definitely not the end of the story, and some of the content may even be short-lived (e.g., if you get rid of all promote beyond RingElement and have this new syntax map(R,S)= then setupPromote can probably go) but I think it's useful to have this as a first step.

[mahrud]

Yes, I think it's great to have this for now.

[mahrud]

Could you push https://github.com/Macaulay2/M2/pull/3519/commits/bbc9f099484946762ef4f667a860be286665a891 to your branch? Or I can do it if that's okay.

[pzinn]

please do

[d-torrance]

@pzinn: Do you have a suggestion for the changelog entry for these promotion updates?

[mahrud]

This PR broke this:

R = CC_53[x,y]
sub(0 * vars R, CC_53)

[mahrud]

@pzinn @d-torrance could you make sure this is either fixed or this PR is reverted before the release until the issue can be fixed?

[d-torrance]

What's the bug? I'm getting the following on the release branch (which is on top of development):

i1 : R = CC_53[x,y];

i2 : sub(0 * vars R, CC_53)

o2 = | 0 0 |

                1         2
o2 : Matrix CC    <-- CC
              53        53

Isn't that the expected result?

[pzinn]

can't reproduce the bug either. as to the blurb:

• allow promotion and lifting between two arbitrary rings using setupPromote and setupLift.

[mahrud]

I would maybe prefer not advertising setupPromote and setupLift yet, since we may switch to different syntax soon?

[mahrud]

This is what I'm getting:

i1 : R = CC_53[x,y]; sub(0 * vars R, CC_53)
stdio:1:19:(3): error: no method found for applying promote to:
     argument 1 :  | x y | (of class Matrix)
     argument 2 :  R
     argument 3 :  CC
                     53

[mahrud]

Ah never mind, I think it's a bad mixed reaction of two changes.

[pzinn]

fine, something more generic then: relax the restriction on promotion/lifting so one can promote/lift between any two rings, and apply this to fraction fields and tensor products.