Promote #3494

pzinn · 2024-09-23T07:53:54Z

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.

mahrud · 2024-10-03T21:43:12Z

M2/Macaulay2/m2/exports.m2

@@ -871,7 +871,7 @@ export {
 	"left",
 	"length",
 	"lift",
-	"liftable",
+	"isLiftable",


This list is sorted, so this line should be next to other "is" symbols.

mahrud · 2024-10-03T21:48:03Z

M2/Macaulay2/m2/newring.m2

@@ -11,6 +11,21 @@ needs "matrix2.m2" -- for lift
 -- new polynomial ring or quotient ring from old --
 ----------------------------------

+-- automate promotion
+promoteFromMap = method()


Maybe call this setupPromotionMethods or something like that?

And perhaps commonEngineRingInitializations should call it, similar to the lift methods?

right, that was part of the idea that it could replace some of the existing code. I haven't looked into it yet.

how about just setupPromote? so it's not too long

Up to you. My plan is to eventually allow the notation map(R, S) = f.

mahrud · 2024-10-03T21:52:23Z

M2/Macaulay2/m2/quotring.m2

-     if liftable(m,ZZ) then lift(m,ZZ) else 0))
+     lift(m,ZZ,Verify=>false) ?? 0))


Not sure if I understand the motivation in this change. I like ?? and ??= in certain situations (e.g. caching) but I'm concerned that in places like this it might make debugging more difficult. For instance, I imagine we would like to see an error if isLiftable returned true but lift failed here, because that would be a bug and an error would help us locate it, but ?? hides the error here and silently returns zero.

in general I would agree with you but here's a pretty clear-cut case where we're basically calling lift twice for no reason.

I'm actually a little confused about what's happening in this code, but sure.

On a second thought, basically any time someone calls isLiftable, a potential call to lift is on the horizon, no?

mahrud · 2024-10-03T21:54:37Z

M2/Macaulay2/m2/rationals.m2

 lift(QQ,QQ) := opts -> (r,QQ) -> r
-liftable(QQ,QQ) := (QQ,QQ) -> true
+isLiftable(QQ,QQ) := (QQ,QQ) -> true


Why does this line even exist? We don't have isLiftable(ZZ,ZZ), etc.. Maybe there should be isLiftable(Ring, Ring) := (R, S) -> R === S that is used as a last resort if R and S don't have an isLiftable method installed?

you're right, this line is useless I believe.
right now for most (all?) rings we have lift(R,R):=o->(x,R)->x so isLiftable(R,R) will give true without any need to override the default behaviour.

pzinn · 2024-10-03T22:03:43Z

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

mahrud · 2024-10-03T22:09:35Z

M2/Macaulay2/m2/newring.m2

+     RS := AB/image fg;
+     promoteFromMap map(RS,R,(vars AB)_{0 .. m-1});
+     if S =!= R then promoteFromMap map(RS,S,(vars AB)_{m .. m+n-1});


Is it natural that when R == S we favor the first ring map over the second?

yeah this is annoying. no there isn't. in fact there are 3 choices here:

favour the first

favour the second

decide that it's ambiguous and not call promoteFromMap at all.

I'm not sure what's best.

I wonder if there's a general lesson to be learned from the way NormalToricVarieties handles natural maps:

i1 : needsPackage "NormalToricVarieties"; i2 : X = toricProjectiveSpace 1; i3 : Y = toricProjectiveSpace 2; i4 : Z = X ** Y; i5 : Z_[0] o5 = | 1 | | 0 | | 0 | o5 : ToricMap Z <--- X i6 : Z_[1] o6 = | 0 0 | | 1 0 | | 0 1 | o6 : ToricMap Z <--- Y i7 : Z^[1] o7 = | 0 1 0 | | 0 0 1 | o7 : ToricMap Y <--- Z i8 : Z^[0] o8 = | 1 0 0 | o8 : ToricMap X <--- Z

I wonder if we could have a notation for specific ring maps. In particular, if X === Y then Z_[0] and Z_[1] are still clearly distinguished.

mahrud · 2024-10-03T22:12:56Z

M2/Macaulay2/m2/ringmap.m2

@@ -85,7 +83,7 @@ map(Ring, Ring, Matrix)  := RingMap => opts -> (R, S, m) -> (
 		    " into a degree of length ", toString degreeLength R);
 	       opts.DegreeMap
 	       )
-	  else if workable (() -> promote({},S,R)) then (d -> first promote({d},S,R))
+	  else if (pr:=lookup(promote,List,S,R)) =!= null then (d -> first pr({d},S,R))


Huh, I didn't know promote(List,S,R) is for promoting the degree till now ...

For the record I fixed this line because it could create infinite loops. It pretty much never occurred before because there were few promote, but now with the added flexibility of promoteFromMap (or whatever we're going to end up calling), it can easily happen.

I think I would be in favor of deprecating promote(List, Ring, Ring) and instead given a ring map f make use of f.degreeMap and f.degreeLift.

Or maybe just degreeGroup f defined as degreeGroup RingMap := f -> f.degreeMap, which is similar to how NormalToricVarieties supports picardGroup ToricMap to give the map of Picard groups functorially.

mahrud · 2024-10-03T22:15:04Z

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 · 2024-10-03T22:36:32Z

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:

M2/M2/Macaulay2/m2/basis.m2

Lines 200 to 203 in 1fd2382

    
           -- TODO: check that S is compatible; i.e. there is a map R <- S 
        
           -- perhaps the map should be given as the option instead? 
        
           S := if opts.SourceRing =!= null then opts.SourceRing else R; 
        
           phi := map(R, S);

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 · 2024-10-03T23:52:31Z

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 · 2024-10-05T10:41:05Z

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 · 2024-10-05T14:01:04Z

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

pzinn · 2024-10-06T00:37:06Z

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

mahrud · 2024-10-06T03:01:53Z

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 · 2024-10-06T03:29:13Z

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 · 2024-10-06T03:50:36Z

Okay, then don't rename liftable.

pzinn · 2024-10-06T04:05:23Z

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 · 2024-10-06T04:53:11Z

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 · 2024-10-06T05:58:53Z

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 · 2024-10-06T07:15:50Z

reverting liftable -> isLiftable 7689295 e9e77da a7e9349

mahrud · 2024-10-07T14:22:00Z

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

pzinn · 2024-10-07T22:44:28Z

I think it should be ready for this release.

pzinn · 2024-10-08T11:50:55Z

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

mahrud · 2024-10-09T22:18:03Z

M2/Macaulay2/packages/Macaulay2Doc/functions/setupLift-doc.m2

+--- author(s): PZJ
+--- notes: 
+
+undocumented methods setupLift


Is there a reason you undocument these rather than just list the method keys as secondary keys?

If it's because you get weird documentation errors, look at the Synopsis keyword used in the documentation of preimage, for instance.

mostly out of laziness. Also, some of the methods are really meant for internal use only, so I need to decide whether to document those or not.

Yeah, I would vote to keep both of these methods unexported for now (of course you can do importFrom_Core "setupPromote" in your own package to use it).

When I said "some of the methods are really meant for internal use only", I did not mean that setupPromote and setupLift were for internal use only. I meant some of their methods (as listed by say methods setupPromote) are.

Well you've listed two things under Usage, so they should be listed as keys.

mahrud · 2024-10-09T22:20:22Z

M2/Macaulay2/packages/Macaulay2Doc/functions/setupLift-doc.m2

+ Key
+  setupLift
+ Headline
+  automatic lift from one ring to another


In what sense is this "automatic"? Seems like the purpose of this function is to manually setup lifting method.

oops. copy-pasted too quickly the setupPromote doc.

My complaint applies there, too. How is that automatic?

it is automatic in the sense that you don't need to invoke promote explicitly, see the examples provided for setupPromote

mahrud · 2024-10-09T22:27:32Z

M2/Macaulay2/m2/enginering.m2

+    -- promote(Matrix,R,S) :=
+    -- promote(MutableMatrix,R,S) := -- doesn't work, cf https://github.com/Macaulay2/M2/issues/2192


I'm confused about these commented lines. Don't you have these exact methods implemented two lines below?

yes, but not as S ** M

Why would it be? That wouldn't even work because Ring ** Matrix calls Ring ** Module which then calls promote(Matrix, Ring), so it would be a loop.

M2/Macaulay2/packages/Macaulay2Doc/doc5.m2

mahrud · 2024-10-11T15:04:01Z

M2/Macaulay2/editors/emacs

This hash changed again.

this is a pain, we should set up the emacs stuff differently so it doesn't occur...

mahrud · 2024-10-11T15:22:11Z

M2/Macaulay2/packages/Macaulay2Doc/functions/setupPromote-doc.m2

+ Key
+  setupPromote
+ Headline
+  automatic promotion from one ring to another


Your force push made my comment disappear, but still, I don't think this headline describes what this function does. If you don't want to remove "automatic", at least add "set up" to the beginning? Also:

list at least one method key

maybe link to this node from promote? (and similarly from lift to setupLift)

will rewrite doc

mahrud · 2024-10-11T15:27:13Z

M2/Macaulay2/m2/enginering.m2

@@ -466,6 +503,8 @@ swap = (x,y) -> (y,x)
 promoterightexact   = swap @@ promoteleftexact   @@ swap
 promoterightinexact = swap @@ promoteleftinexact @@ swap

+isPromotable = (R,S) -> lookup(promote,R,S) =!= null


Should be defined next to promote?

mahrud · 2024-10-11T15:31:26Z

M2/Macaulay2/m2/enginering.m2

+    lift(R,S) := opts -> (a,S) -> if opts.Verify then f a else try f a;
+    lift(List,R,S) := opts -> (m,R,S) -> apply(m,degmap);
+    lift(Module,R,S) := opts -> (M,R,S) -> S ** M;
+    lift(Matrix,R,S) := opts -> (m,R,S) -> map(lift(target m,S),lift(source m,S),applyTable(entries m,x->lift(x,S)));
+    lift(MutableMatrix,R,S) := opts -> (m,R,S) -> mutableMatrix applyTable(entries m,x->lift(x,S));


Do we have any lift methods of matching e.g. lift(Module, R, S) that isn't defined here automatically? Because if not, I think all of these should just be e.g. lift(Module, Ring, Ring), and we can rename lift(x,S) to something else that is defined per ring.

good question. lift(Module,...) only works for free modules anyway, it seems the definition I give could work universally. that being said it's not how it's done for the usual internal lift, and I don't dare to change too much.

implement isPromotable

e717720

pzinn marked this pull request as draft September 23, 2024 07:54

pzinn force-pushed the promote branch from d5fb23b to af33400 Compare September 23, 2024 11:48

pzinn force-pushed the promote branch from 968ae80 to cc75826 Compare October 3, 2024 21:34

mahrud reviewed Oct 3, 2024

View reviewed changes

pzinn force-pushed the promote branch 2 times, most recently from a87434d to a7e9349 Compare October 4, 2024 13:10

pzinn force-pushed the promote branch 4 times, most recently from 04b5a89 to 53983f1 Compare October 7, 2024 06:48

pzinn force-pushed the promote branch from 53983f1 to 68ee661 Compare October 7, 2024 10:10

pzinn force-pushed the promote branch 2 times, most recently from 9ce1531 to 1ff3f83 Compare October 7, 2024 23:16

pzinn added 2 commits October 8, 2024 10:50

setupPromote, setupLift, use in tensor products, selectVariables

28bee55

minor liftabale changes

309161a

pzinn force-pushed the promote branch from 1ff3f83 to cf5b51d Compare October 8, 2024 04:31

pzinn marked this pull request as ready for review October 8, 2024 04:31

mahrud reviewed Oct 9, 2024

View reviewed changes

M2/Macaulay2/packages/Macaulay2Doc/doc5.m2 Show resolved Hide resolved

doc and tests for promote, lift update

d1023ef

pzinn force-pushed the promote branch from cf5b51d to d1023ef Compare October 10, 2024 23:01

mahrud reviewed Oct 11, 2024

View reviewed changes

		if liftable(m,ZZ) then lift(m,ZZ) else 0))
		lift(m,ZZ,Verify=>false) ?? 0))

		-- promote(Matrix,R,S) :=
		-- promote(MutableMatrix,R,S) := -- doesn't work, cf https://github.com/Macaulay2/M2/issues/2192

Promote #3494

Are you sure you want to change the base?

Promote #3494

Conversation

pzinn commented Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pzinn commented Oct 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahrud Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahrud commented Oct 3, 2024

mahrud commented Oct 3, 2024 • edited Loading

pzinn commented Oct 3, 2024

pzinn commented Oct 5, 2024

mahrud commented Oct 5, 2024

pzinn commented Oct 6, 2024

mahrud commented Oct 6, 2024

pzinn commented Oct 6, 2024

mahrud commented Oct 6, 2024 • edited Loading

pzinn commented Oct 6, 2024

mahrud commented Oct 6, 2024 • edited Loading

pzinn commented Oct 6, 2024

pzinn commented Oct 6, 2024

mahrud commented Oct 7, 2024

pzinn commented Oct 7, 2024

pzinn commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahrud Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahrud Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pzinn commented Sep 23, 2024 •

edited

Loading

mahrud Oct 3, 2024 •

edited

Loading

mahrud commented Oct 3, 2024 •

edited

Loading

mahrud commented Oct 6, 2024 •

edited

Loading

mahrud commented Oct 6, 2024 •

edited

Loading

mahrud Oct 11, 2024 •

edited

Loading

mahrud Oct 11, 2024 •

edited

Loading