Some contributions to the PushForward package#4176
Conversation
|
Thanks for the contribution! @Devlin-Mallory and I have also been thinking about rewriting parts of this package (e.g. to support sheaves and multigradings), so I'm glad to see others working on it as well. I'll take a look soon. Do your changes resolve any of the existing issues about this package, like #3165, #3284, or #3887? It's worth adding tests and a link to the corresponding issue if so. Regarding formatting, note that the standard indent size for M2 code is 4 spaces (optionally replacing every 8 by a tab, but not strictly necessary). Also, note that including even a comment like inside the tests causes the tests to take longer (because tests matching certain words like "restart" can't use a quicker verification function). |
|
@mahrud amazing thank you! I did not inspect the issue backlog but it seems likely there are some overlapping concerns with the issues you shared. I'll take a look! After you get a chance to review I'll also take a careful pass to see that I've followed indentation conventions. The whitespace changes that I mentioned were mostly around adding some nesting for readability in if / then / else clauses and things like that. The restart and needs package boilerplate in the tests I added was cargo culted from other tests and I don't have any special concerns around removing I if I the test ls run and pass. I'll try it out. |
| ideals := {lastIdeal}; | ||
|
|
||
| -- iteratively gather the defining ideals of baseRings of R | ||
| while ring lastIdeal =!= R do ( |
There was a problem hiding this comment.
Since lastIdeal is defined initially as ideal R, wouldn't ring lastIdeal be R from the beginning?
There was a problem hiding this comment.
ideal R gives the ideal used to define R as a quotient ring so it is an ideal in a ring mapping onto R.
There was a problem hiding this comment.
You might ask whether this method could just be replaced by ideal flattenRing R because it seems like it ought to give the same result. In practice I think I found that it did the right thing in cases where flattenRing didn't but I have lost track of the examples that led me down this path and I will actually revisit this to see if flattenRing won't do the trick after all.
There was a problem hiding this comment.
Are you trying to push forward to the polynomial ring and do the computation there? Then flattenRing might work well:
i1 : R1 = kk[x,y]/(x^2-y^2);
i2 : R2 = R1[s,t]/(s*x+t*y)
i3 : ideal R2
o3 = ideal(x*s + y*t)
o3 : Ideal of R1[s..t]
i4 : ring ideal R2
o4 = R1[s..t]
o4 : PolynomialRing
i6 : ideal first flattenRing R2
2 2
o6 = ideal (x - y , s*x + t*y)
o6 : Ideal of kk[s..t, x..y]
i8 : ring ideal first flattenRing R2
o8 = kk[s..t, x..y]
o8 : PolynomialRingThere was a problem hiding this comment.
D'oh -- I made the comment above before drinking my morning coffee! Wasn't thinking about quotient rings.
If you do decide that flattenRing won't work and to stick with a while loop, then using while ... list would be more efficient. Any time we use append inside a loop like this, it creates a brand-new list. Instead, we could do something like:
while (lastIdeal := ideal R; ring lastIdeal =!= R) list (R := ring lastIdeal; lastIdeal)There was a problem hiding this comment.
Or just use this:
i3 : R2.baseRings
o3 = {ZZ, kk, kk[x..y], R1, R1[s..t]}There was a problem hiding this comment.
Although I'd like this to be simple I think that what I want to do is what the code does as is. That is, gather relations from ideals used to define R in a tower of rings so that we can inspect that all of the generators of R are chopped down enough to make R finite over it's coefficient ring.
In practice, flattenRing seems to not support many of the cases that this covers. Replacing the use of liftDefiningIdeals with
ideal flattenRing(R, CoefficientRing => R, Result => 1);
and running tests surfaces error: unable to flatten ring over given coefficient ring in about half of the test-cases. Empirically, this seems to match my experience working with flattenRing interactively although I have not tried to carve out a clear understanding of which rings it seems to support and which it does not.
I also explored rewriting based on appropriately iterating through some segment of R.baseRings but this was finicky and seemed to have little advantage over the current approach.
I am rewriting given the guidance around while list do which is very helpful! (although indeed I had to fuss with the logic a bit - while loops are so weird somehow 😭 )
There was a problem hiding this comment.
I wonder if the right thing to do (now or in a separate change) would be to augment what flattenRing does with logic like liftDefiningIdeals has here ...
|
I am pushing another patch with an update to the |
|
Added some examples to PR description to clarify what is fixed here. |
| mapf = if isHomogeneous f then (b) -> ( | ||
| (mons,cfs) := coefficients(b, Monomials => matB); | ||
| try | ||
| lift(cfs, A) |
There was a problem hiding this comment.
This is the bit of this change that I am most uncertain about and would love some guidance on. I am shaky on how to think about how to correctly model a pushforward module in the presence of multi-grading on the target ring. I ran into some cases of this with my testing and thought that adding a fallback to strip grading (copied from the inhomogeneous case) was reasonable but after continuing to read and think about this I am less sure.
The examples i ran into were things like
R = ZZ/101[a]/a^2
S = R[b]/b^2
without setting Join => false, S is multi-graded and the pushforward function that maps elements of S to elements of the pushforward R^2 will throw an error complaining that the degree cannot be lifted.
There was a problem hiding this comment.
@mahrud @d-torrance do you have any thoughts here? Specifically, is it reasonable to fallback to an inhomogenous map from the ring to its push-forward module in case the homogenous lifting fails or would it be preferred to raise an error and fail as happens without this change?
There was a problem hiding this comment.
Ah reading your initial comment more carefully - and with a few more days of letting things soak for me - I see that this is likely very tied up in your hope for supporting multi-gradings in this package. I don't have the clearest view of what the optimal behavior is but would be very interested to understand the desired functioning of push forward in the presence of multi-grading from your pov.
|
@mahrud @d-torrance can you help frame for me what are the next steps for this PR? I am totally uncalibrated (for this project) on what type of discussion / collaboration to expect on a PR like this and on what time scale this will happen! |
|
It's a busy time of the semester for me and I haven't had any time to look yet. I'll hopefully have more time in the next 2 weeks. Usually after the review we ping the package authors to make sure they agree with the changes before merging. |
|
Makes perfect sense thank you for clarifying the process! |
|
I have updated this PR to also include the construction of methods that perform the bijections between a module N and it's pushforward. I'd really appreciate some feedback on:
I don't intend to augment the current PR with any more functionality but will rather wait to work through review before proceeding to next steps with the workstream I have in mind. |
| /// | ||
|
|
||
| -- test 21 | ||
| TEST /// |
There was a problem hiding this comment.
when running tests locally this test and the two that follow it (21, 22, 23) consistently fail and then succeed on retry - so that the test suite passes but these require two attempts.
It seems to me like this is maybe a result of some sort of environment bleed over between test runs but i see this even when running a single test case as in check_21 "PushForward".
There was a problem hiding this comment.
Mystery solved. I had failed to define kk in these tests. For ease of interactive testing and development I had defined kk = ZZ/101 in my init.m2 file...
Based on observed behavior I suspect that there is some asymmetry between how tests are run on the first attempt and how they are retried. In particular, is it the case that the first run does not load init.m2 but that the retry passes through a restart that loads it...
Anyway I have added the definition and this all works as expected now.
There was a problem hiding this comment.
By default, tests are run using capture, which runs M2 code w/o firing up a brand new process. If that fails, then we start up a new process and try again. Neither attempt should be loading init.m2 -- it should be like running M2 -q.
There was a problem hiding this comment.
Fascinating I see. I'm at a loss to explain what I was seeing locally then!
There was a problem hiding this comment.
Empirically it looks to me VERY much like the "try again" loads init.m2. On my local branch I commented out both the line from check_21 defining kk and the line in my init.m2 defining kk. Running the check resulted in a failure:
i1 : check_21 "PushForward"
-- warning: reloading PushForward; recreate instances of types from this package
-- capturing check(21, "PushForward") -- capture failed; retrying ...
-- running check(21, "PushForward") -- running failed
-- error log: /tmp/M2-33393-0/0.tmp:0:1:(3):[16]: stdio:5:6:(3):[1]: error: no method for adjacent objects:
-- kk (of class Symbol)
-- SPACE [a] (of class Array)
--
-- input code: /usr/local/share/Macaulay2/PushForward.m2:1103:5-1121:3
*** error: M2 exited with status code 1 -- 1.10622s elapsed
=================================================================================================================================================================================================================================
-- Summary: 1 test(s) failed in package PushForward:
-- warning: reloading PushForward; recreate instances of types from this package
-- Test #21. /usr/local/share/Macaulay2/PushForward.m2:1103:5-1121:3
stdio:1:8:(3):[1]: error: repeat failed tests with:
check({21}, "PushForward")
Then uncommenting the def in init.m2 and running again results in success (on retry):
i2 : check_21 "PushForward"
-- capturing check(21, "PushForward") -- capture failed; retrying ...
-- running check(21, "PushForward")
I am not TOO concerned about this but here we are talking about so just want to clarify what I see!
| -- without pruning | ||
| M = pushFwd(N, NoPrune => true); | ||
| (mapf, mapb) = M.cache.pushMethods; | ||
| assert(n - mapb mapf n == 0) |
There was a problem hiding this comment.
You can see that the test cases I have added consistently compare these values by asserting that a difference is 0 rather than asserting equality directly. In fact the assertion of equality fails and I have been unable to understand why that would be when their difference is 0.
There must be some hidden state that is incorporated into the equality check that is bypassed in the case when all coefficients are 0 or something? If there is some context you have that could help explain this or contextualize whether this seems like a concern I'd appreciate it!
|
This is next on my TODO list, sorry for the delay! |
|
No problem it has been a productive delay for me (and for the PR)! |
|
I can't quite find the line causing this problem, but I think there's a small issue arising from the caching of |
|
|
||
| -- compute pushFwd along a ring map | ||
| pushFwd = method(Options => {NoPrune => false}) | ||
| pushFwd Ring := Sequence => o -> B -> pushFwd(map(B, coefficientRing B), o) |
There was a problem hiding this comment.
I'm unsure how I feel about having pushFwd R (or M, etc) be a synonym for the pushforward along the inclusion of the coefficient ring. I would lean towards always having an explicit ring map in the argument for pushFwd, though I'm curious how others feel.
There was a problem hiding this comment.
One argument to remove these synonyms based on my experience testing is that while I often make use of the implicit inclusion of the coefficient ring, I always have to stop and think about whether that is the right behavior. That is, the synonym does not really provide too much encapsulation of complexity. You still have to consciously think about it. Maybe its that from the point of view of a module the pushforward along the coefficient ring inclusion isnt really natural so you always have to think about it?
It probably only really applies to the simplest cases anyways. Curious if you can say more what shape your concerns have as well.
There was a problem hiding this comment.
I think for me the concern is partly one of mathematical grammar, i.e., that pushforward should be a functor produced from a ring map, rather than a ring itself (though perhaps implicitly all macaulay2 rings should be thought of as relative objects over their coefficient rings), and partly that in many (most?) cases rings are not finite over their coefficient rings so this behavior is really only defined in a special subset of cases.
There was a problem hiding this comment.
Ooooh that is interesting. From that POV, all of these methods as uncurried versions of a single pushFwd: RingMap -> Functor and such a Functor can then be applied to any number of different types of objects. That makes sense to me as the right way to think about this and it does some nice to have the implementation nudge users towards it.
pushFwd already has the implicit map(R, coefficientRing R) and I feel pretty strongly that pushforward and pushFwd ought to have the same semantics wrt this question. How do you understand the scope of a proposal to removal this interface and how to validate that it is the right change to make?
There was a problem hiding this comment.
I have some local WIP that I think makes me more interested in requiring an explicit ring map when pushing forward - removing the implicit map(R, coefficientRing R).
My primary goal in this stream of work is to get to a place where I can compute Ext of modules over skew-symmetric polynomial / quotient rings. One intermediate goal is to fix the computation of Hom(M, N) when M, N are modules over an exterior algebra S. Currently, macaulay2 returns an S-module as the answer which we know cannot be right since in general Hom over non-commutative rings does not have an S-module structure! Further evidence can be found in this code snippet:
S = kk[a..d, SkewCommutative => true];
I = module ideal {a*b +c };
H = Hom(I, I);
homomorphism' id_I
o73 = 0
:/ not a great situation when the identity map is not in your set of homomorphisms lol
The approach I am taking is to compute pushforwards along a map from a commutative ring, f: R -> S
pM = pushFwd(f, M);
pN = pushFwd(f, N);
H = Hom(pM, pN);
and to realize Hom(M, N) as the R-submodule of H that commutes with (R-linear!) maps "multiplication by s" for all s in S. The fact that you get back a module over a different ring feels more surprising in the context of computing Homs than it does in the case of pushFwd where you are of course primarily in the business of changing rings. For me, if I am computing a pushFwd it seems more possible to guess what is done without an explicit map than it does when I am computing a Hom. Making the map explicit reduces surprise in this case.
Then I want to enforce these semantics in the pushFwd case because the semantics of whether a ring map is required or not ought to be the same!
(this issue of requiring the ring map also shows up when supporting homomorphism' by the way. Given a matrix over S that represents an S-linear map from M -> N, we can produce its element in a hom-set but we need to know which one!)
Excellent thank you! I also see this issue but the patch I have to update edit: oop no I think I had not fixed this problem yet - unsurprisingly given your report it is related to accessing the cache in the RingMap case. Looking at this now. |
| @@ -0,0 +1,123 @@ | |||
| restart | |||
There was a problem hiding this comment.
This is a collection of scripty things and comments that was in the footer of PushForward.m2 after the beginDocumentation call. I think placing it in a text file like so is reasonable if we want to keep it around.
There was a problem hiding this comment.
You should keep them at the end of the package file.
There was a problem hiding this comment.
OK I will put them back at the bottom of the package file. Are the commands here executed at some point?
There was a problem hiding this comment.
I guess I missed adding back a load bearing end before this section in the main package file. I'll try that and see if that fixes the build.
| setPushforwardCache'(pfB, (a) -> ( | ||
| -- coerce to matrix over B | ||
| coeffs := map(module B, B^(numcols a), auxmapb * a); | ||
| -- this try is to handle the case where coeffs has a RingMap attached |
There was a problem hiding this comment.
@Devlin-Mallory this is where the issue that you reported was getting raised. I do not understand why sometimes the matrix coeffs carries along a RingMap and sometimes it does not. In general I think it is virtuous to not clobber and recreate a matrix via matrix entries - since there could be valuable degree information (is this correct?) but as a fallback behavior it seems reasonable.
There was a problem hiding this comment.
I also don't understand the behavior of coeffs there. Your fallback seems reasonable; you're right that there can be degree information in the matrix, but by leaving the second argument blank Macaulay2 will try to grade the source such that the map is homogeneous anyways (which is I guess what you're using already).
|
@Devlin-Mallory - Added a fix for the bug that you pointed out. I will comment where the fix was. I have added documentation and split tests and docs for the PushForward package into separate auxiliary files. This is getting polished quite nicely. Along with any other feedback that you may have in mind there are a couple more things that I think ought to be included in this. I am sorry that this keeps having little things added to it!
|
|
No problem on adding things - we keep asking for more stuff, and it seems like a nice set of features to have! Another bug I'm running into, possibly related to the one yesterday: FS = map(S,S, {x^3,y^3,z^3})
pushforward'(gens first pushFwd FS) -- works well, returns the matrix of generators
R = (ZZ/3)[x,y,z]/(x*z-y^2)
FR = map(R,R, {x^3,y^3,z^3})
pushforward'(gens first pushFwd FR) -- "expected an element of a module of the form pushFwd(N)"I'm not sure what the issue is exactly - I thought at first it was maybe a distinction between polynomial rings and quotient rings, but other quotient rings of S don't have the same problem. (The same problem also occurs if I replace the source of FR by another ring, so I don't think it's coming from the fact that FR has the same source and target.) |
|
I believe the issue in your example is that I see the same error with your example but this amendment works for me as expected: actually more detail: you left off the definition of philosophical aside: In fact this is what I would have expected |
| if isInclusionOfCoefficientRing f then | ||
| isModuleFinite target f | ||
| else | ||
| pushFwdRingHelper(f, ErrorOnFailure => false) =!= null |
There was a problem hiding this comment.
This ErrorOnFailure trick, and functions returning different types in general, is not good practice. You can use try pushFwdRingHelper f then true else false to check if it fails or not.
There was a problem hiding this comment.
Indeed I agree that this is a bit smelly. Your suggestion avoids this but has the drawback that it may catch errors besides the ones that are raised by the validation loop. IIUC there is no way to catch specific errors or introspect error messages in macaulay2 at this time, is that right?
If you think that it is better to preserve a simple interface in this helper method at the expense of possibly misleading the user with a bad error message, I am happy to change this. If you think avoiding both of these is preferred I can chop up the helpers more to get non-overloaded method interfaces AND good error semantics at the expense of having more (potentially weird) helper methods.
It does seem good to me to avoid the duplication of the large block of shared code between the previous isFinite1 and pushAuxHgs (in general I am wary of overuse of avoiding code duplication but i this case it does seem that the concerns are IDENTICAL).
There was a problem hiding this comment.
e.g. it is formal and trivial to crack this helper open like so:
prepareAux = (f) -> (
...
(R, A, B, I, m, n, rels, xvars, yvars, varsA, varsB)
)
validateAux = (R, m, n, FB, rels) -> (
-- skew variables don't show up as explicit relations but are nilpotent so
-- don't need to be checked here.
skewInds := set(if isSkewCommutative FB then FB.SkewCommutative else {});
notFinite := any(
1..n,
i -> (
not member(i - 1, skewInds) and
ideal(sub(gens rels, matrix {{(i-1):0, 1_R, (m+n-i):0}})) != ideal(1_R)
)
);
if notFinite then error "not a finite map";
)
doAux = (R, A, B, I, rels, xvars, yvars, varsA, varsB) -> (
...
(matB, mapf)
)
and then the two callsites can do either
(R, A, B, I, m, n, rels, xvars, yvars, varsA, varsB) := prepareAux(f);
try(validateAux(R, m, n, FB, rels) then true else false
or
(R, A, B, I, m, n, rels, xvars, yvars, varsA, varsB) := prepareAux(f);
validateAux(R, m, n, FB, rels);
doAux(R, A, B, I, rels, xvars, yvars, varsA, varsB)
but in this form leaking all those intermediate variables really sucks. I am not sure how often bespoke data types are used for this sort of thing in macaulay2. I don't mind in general introducing something like a
pushFwdArtifacts {
-- data type (hashmap?) that encapsulates all these variables from the caller
-- that is stitching the prepare / validate / perform sequence that they want
}
prepare could return one of these and validate / do could just accept it and unpack what they need from it.
There was a problem hiding this comment.
I agree that when possible, avoiding try is best. It's hard for me to tell why you separated the functions the way you did, since there are a lot of changes in the PR now.
There was a problem hiding this comment.
That's fair. Previously this method was used for isModuleFinite in the case when the map is not the inclusion of the coefficientRing:
isFinite1 = (f) -> (
A := source f;
B := target f;
matB := null;
mapf := null;
pols := f.matrix;
(FA, phiA) := flattenRing A;
iFA := ideal FA;
varsA := flatten entries phiA^-1 vars FA;
RA := try(ring source presentation FA) else FA;
(FB, phiB) := flattenRing B;
iFB := ideal FB;
varsB := flatten entries phiB^-1 vars FB;
RB := try(ring source presentation FB) else FB;
m := numgens FA;
n := numgens FB;
pols = pols_{0..(m-1)};
R := try(tensor(RB, RA, Join => false)) else tensor(RB, RA, Join => true);
xvars := (gens R)_{n..n+m-1};
yvars := (gens R)_{0..n-1};
iA := sub(ideal FA,matrix{xvars});
iB := sub(ideal FB,matrix{yvars});
iGraph := ideal(matrix{xvars}-sub(pols,matrix{yvars}));
I := iA+iB+iGraph;
inI := leadTerm I;
r := ideal(sub(inI,matrix{yvars | splice{m:0}}));
for i from 1 to n do
if ideal(sub(gens r,matrix{{(i-1):0,1_R,(m+n-i):0}}))!=ideal(1_R) then
return false;
true
)
the helper method pushAuxHgs shared a block of code that was identical to this wth the single exception being that an error is raised instead of false being returned. The changes to introduce a helper that owns this logic were an attempt to avoid this duplication.
There was a problem hiding this comment.
Changed this pushFwdRingHelper method to use golang style error passing. This lets callers decide what to do based on the presence of absence of an error string in the second slot of the return type. I wouldn't expect this type of interface to be acceptable for an exported function but for internal package methods that want the flexibility it provides it seems fine to me. WDYT.
|
There are many comment threads but just leaving a top level comment to clarify that I am not aware of any comments requiring response or actions from me and I think this is ready for review. |
4 participants
This PR includes changes to the PushForward package to fix some buggy behavior and support a broader class of ring maps. Itemized changes include:
These are accomplished primarily by making modifications to the
isModuleFiniteandmakeModulemethods. In addition some tweaks are made to the control flow around the construction of the accessor map from a ring to it's push-forward related to failure of lifting in multi-degree situations.There are also some more trivial changes to code formatting I would recommend hiding white space changes when reviewing this PR.
For context, the changes I am making are in service of a high level goal to allow computation of Ext for modules over SkewCommutative polynomial rings.
A sketch of future work related to this includes things like:
Support functoriality of pushFwd with respect to module homomorphisms:given an S-hom f: M → N we should get an R-hom pf(f): pf(M) → pf(N)This is my first time contributing a PR to this repo so please help get me on the rails of the right contributor culture if necessary!
Some examples of fixed behavior in this branch
Previously, skew commutative polynomial rings would give
false:Towers of rings handled now. Previously last line would give
false:This example also gives T not module-finite over R previously because of the extra (unnecessary) relation on
a. Once the isModuleFinite calculation was fixed, the pushFwd was incorrectly computed as a free R-module: