Some contributions to the PushForward package

[joel-dodge]

This PR touches nearly every line of the PushForward package. It offers new functionality, bug-fixes and pure refactoring for readability / maintainability but leaves the algorithmic core of the package mostly unchanged. Itemized changes include:

• extends isModuleFinite and pushFwd methods to cover the case of skew symmetric polynomial rings and to better handle relations coming from towers of rings.
• implements isomorphisms in both directions between a module and it's push-forward available via the pushforward and pushforward' methods.
• introduces auxiliary files and separates tests and documentation nodes to their own files.
• renames some internal methods and refactors them to share code where possible.
• implements caching scheme to support pushforward, pushforward' methods and to cache computations of push-forward modules.
• adds test cases to cover all new functionality.

It is recommended to review this PR in github commit by commit although even then the changes are not all easy to read. Moving the auxiliary files in particular seems to have confused git into producing a difficult to read diff. I also recommend turning off whitespace changes in the viewing settings for this PR.

[mahrud]

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

restart
needsPackage "PushForward"

inside the tests causes the tests to take longer (because tests matching certain words like "restart" can't use a quicker verification function).

[joel-dodge]

@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.

[d-torrance]

Since lastIdeal is defined initially as ideal R, wouldn't ring lastIdeal be R from the beginning?

[joel-dodge]

ideal R gives the ideal used to define R as a quotient ring so it is an ideal in a ring mapping onto R.

[joel-dodge]

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.

[mahrud]

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 : PolynomialRing

[d-torrance]

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)

[mahrud]

Or just use this:

i3 : R2.baseRings

o3 = {ZZ, kk, kk[x..y], R1, R1[s..t]}

[joel-dodge]

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 😭 )

[joel-dodge]

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 ...

[joel-dodge]

I am pushing another patch with an update to the listDefiningIdeals method and fixes to remove the harmful test comments and fix some indentation convention violations.

[joel-dodge]

Added some examples to PR description to clarify what is fixed here.

[joel-dodge]

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.

[joel-dodge]

@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?

[joel-dodge]

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.

[joel-dodge]

@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!

[mahrud]

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.

[joel-dodge]

Makes perfect sense thank you for clarifying the process!

[joel-dodge]

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:

• whether it looks like I have overcomplicated the construction or it seems reasonable.
• is the use of the cache for this purpose appropriate?

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.

[joel-dodge]

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".

[joel-dodge]

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.

[d-torrance]

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.

[joel-dodge]

Fascinating I see. I'm at a loss to explain what I was seeing locally then!

[joel-dodge]

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!

[joel-dodge]

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!

[mahrud]

This is next on my TODO list, sorry for the delay!

[joel-dodge]

No problem it has been a productive delay for me (and for the PR)!

[mahrud]

Let's cache this as f.cache.pushforwardMap instead.

[mahrud]

And it should be mentioned in the documentation; e.g. see M.cache.pruningMap.

[joel-dodge]

Interesting, I am surprised so let me clarify my thinking. From a ring map f: A -> B we get a pushforward map from B -> pushFwd B^1. I think caching this on f make sense and I didn't do so in this change just because the map was already being returned directly via the pushFwd method itself.

In this case, we also have maps between B^1 and its pushforward, I called them mapf, mapb here. I think it makes sense to store these on the pushFwd module itself as I am doing here. Storing on f is weird becuase we may of course pushfwd many different modules with the same ring map (as in pushFwd of a matrix even).

I guess that you commented on L129 but your comment was really referring to what we should do with ringmapf defined on L126?

If what I am saying here makes sense, then I am also interested in feedback on how to store mapf, mapb in the cache of pushFwd M in general. Currently they are attached to pushMethods key in the cache as an array. Do you think it would make sense to store them separately side by side? e.g. after this is accepted I have some more changes I am thinking about that would attach more metadata to the cache like:

• module that this was pushed from
• ring map that the psuhfwd was computed with

so am interested in structuring the use of the cache right.

[joel-dodge]

Re: documenation: I held off from writing documentation pending feedback on whether the use of the cache in this way seemed like the right approach.

[joel-dodge]

I ended up caching this on the ring itself under f as a key since I ran into some difficulty in the case where map(R, coefficientRing R) is used implicitly. It seems that, while map(R, coefficientRing R) == map(R, coefficientRing R), different instances of this map are not the same enough to share a cache. They are the same enough that they can be used interchangeably as a cache key though.

Storing it on the ring provides some predictability in where these methods are stored which is nice:

• the stuff backing pushforward / pushforward' is stored on the module / ring containing the elements getting pushed
• pushforward stuff is stored keyed by the ring map (since you can push out along potentially several different ring maps
• pushforward' stuff just needs one key since a given module is the pushforward of exactly one module.

[mahrud]

I need to think about this a bit longer, but I'm wondering if we should introduce a new method, similar to homomorphism and homomorphism', which uses a cached map to perform some operation. Then you don't need to check the type manually. (On that note, you should check type with instance(x, Matrix) instead)

[mahrud]

@Devlin-Mallory what do you think?

[joel-dodge]

Ah yes I see that seems potentially nice. I will also chew on the semantics of this, what it would be called, etc. It seems like if we could get the naming and interface right then this would be a lovely solution. Expecting users to fiddle with the cache is a little smelly right?

[joel-dodge]

e.g. what about introducing a couple more methods on pushFwd

pushFwd(RingMap, ModuleElement) = (f, n) -> m
-- compute `pushFwd module n` if it ha not already been computed
-- module n just has to be finite over source f

pushFwd(RingMap, RingElemet) - (f, b) -> a
-- ring b == source f
-- compute `pushFwd ring b` along f and then give the
-- corresponding element of pushFwd ring b

and maybe another exported method:

pushBack(ModuleElement) = (f, m) -> n
-- m must be an element of a module which is pushFwd of some module along f

questions:

• does it make sense to cache pushFwd modules on f itself to avoid recomputing in the first case? can we use N as a key to lookup pushFwd N in f.cache?
• I am not clear on whether ModuleElement is really a well-defined type. I have been using matrices exhibiting an element of M as an R-point as the canonical way to represent an element of a module: Matrix M <- R^1.
• in the above concern, is it clear how to distinguish pushFwd of a single element from pushFwd of an R-module homomorphism?

[joel-dodge]

Actually, it seems neater to me to introduce new exported methods for mapping elements around. Something like

pushforward(RingMap, Matrix)
pushforward(RingMap, RingElement)
pushback(Matrix)

with semantics as described above (and as you say operating similar to how homomorphism / homomorphism' do supported by caches on the relevant module / ring objects).

[mahrud]

This inconsistency of using NoPrune is quite annoying. I would be in favor a introducing a breaking change to rename this as MinimalGenerators => true/false (with default false). This is wordier but matches several other methods in M2.

[joel-dodge]

From my POV this seems great but I don't have a clear view of how to justify a breaking change. If you can lend me your confidence that this is what we should do I am happy to include it here.

[mahrud]

I think this is doing something like ideal first flattenRing R or presentation first flattenRing R, right?

[joel-dodge]

I think that flattenRing had some behavior that seemed unexpected for one of the toy examples I was working with. I'll try and reproduce what led me to write this by hand and maybe we can sort out what is going on. I'll followup with more detail when I have done so.

[joel-dodge]

Here is a toy example where flattenRing doesn't do quite the right thing:

kk = ZZ/101;
A = kk[a];
R = A[b]/ideal {b^2};
i4 : ideal first flattenRing R
o4 = ideal b^2
o4 : Ideal of kk[b, a]

You can see that flattenRing has somehow gone too far back. We don't need to kill off both b and a in order to be module finite over A. I did play around a bit with the CoefficientRing option to flattenRing but the semantics are not quite right there either.

e.g.

f = map(R, A);
i9 : flattenRing(R, CoefficientRing => source f)
o9 = (R, map (R, R, {b, a}))

just returns R without any flattening at all. In fact maybe that is reasonable in this case but in general I could not figure out how to formulate this with flattenRing to produce the "right" ideal in the "right" ring to perform the analysis that isModuleFinite wants to do.

[joel-dodge]

I gave it another go at using flattenRing directly and I think was more successful than when I initially wrote this. WDYT

[joel-dodge]

I ended up LOVING the suggestion to follow the homomorphism pattern so added it here. I think this is a really nice interface.

If it looks reasonable I will write some documentation. Still happy to switch NoPrune -> MinimalGenerators if you'd like.

[joel-dodge]

@mahrud assuming the build passes I am done making updates pending more feedback on questions i have asked.

[joel-dodge]

Oops I don't think that the PUSHFORWARD key is used anymore so doesn't need to be protected.

[mahrud]

I'm not sure what pushforward' is supposed to be. Could you explain what you're trying to do?

[joel-dodge]

Of course. I called it pushforward' following the convention of homomorphism and homomorphism'. They are mutually inverse maps where pushforward: M -> pushFwd M and pushforward': pushFwd M -> M.

Do you think this thing / thing' naming convention is more confusing in this context than it is in for example homomorphism' and adjoint'? Happy to rename!

(Sorry I should have commented that I had used a different name in the implementation than I had sketched out in comments on this PR)

[joel-dodge]

I'd be happy to jump on a call to explain / discuss anything if that seems like it would be useful.

[joel-dodge]

In case an example would help:

With these methods pushforward and pushforward' we can replace the existing pushFwd(RingMap, Matrix) impl with

pushFwd(RingMap, Matrix) := Matrix => o -> (f, F) -> (
    M := pushFwd(f, source F, o);
    N := pushFwd(f, target F, o);
    map(N, M, pushforward(f, F * pushforward' matrix M_*))
)

The result is I think conceptually quite clear and empirically it is orders of magnitude faster. e.g. on an example i computed on my branch the difference in timing for a matrix whose pushforward is a 47x47 matrix over a field was 50s for the current impl and .2s for this new one.

[joel-dodge]

the semantics of pushforwards are already a little funny here. consider

M = pushFwd N
M' = pushFwd(N, NoPrune => true)
m = M_0
n = pushforward' m -- element of N
pushforward n -- element of M' not M our second pushFwd overwrites the map

[joel-dodge]

OK after some thought I think:

• having pushforward side-effect creating the pushFwd module if it does not already exist seems like nice behavior and I like it.
• this means that there ought to be a cache supported way to avoid recomputing pushforward modules that have already been created. the cache should be aware of all pushfwd create options - currently only NoPrune.
• pushforward should also be aware of the pushFwd create options. Seems fine to me even though i said i didnt like it above.

If you agree, then I am happy to include them here or equally happy to fast follow in a separate PR once this merges.

I also DO think that I like the separate methods for element maps to distinguish them from the module level maps. its true that it is hard to get the names just right to make it clear what is different but there IS something different about them and id rather call attention to the difference even if it is not obvioys what it is. of course the documentation will make it clear and the method sigature will help too.

[joel-dodge]

I went ahead and implemented the cache with an initial stab at new semantics for pushforward/pushforward' maps involving options but I think that these feel very strange to me. It feels unintuitive to have to specify pushFwd module creation options when mapping elements around and I'd like to update this to have an interface that feels more natural (while also being fully expressive).

I would love some feedback on this proposal:

pushforward method does NOT accept the NoPrune option but instead has the following semantics:

• If there is exactly one PUSHFORWARDS method cached involving f, then pushforward(f, n) will apply that map.
• if there are no PUSHFORWARDS cached for f, then pushforward(f, n) will create a pushFwd module with the default options and n will be pushed to that module. You cannot side-effect creating a non-default-options pushFwd using pushforward!
• if there is more than one PUSHFORWARDS cached for f, then pushforward(f, n) will raise an error that you have to specify the module to pushforward to

and following the last point

• pushforward will have a new method with interface pushforward(Module, Matrix) = (M, n) -> .... that will push n to the module M which must be a precomputed pushfwd of module n.

I think these semantics feel intuitive and nice. They gracefully work as expected in the typical case of using default options and not having multiple pushforwards involving different NoPrune options. They degrade to a still reasonable behavior of requiring explicitly creating a pushFwd module in advance if you want non-default pruning behavior but still allow the natural "push this element along THE pushforward map" behavior. If you want to manage multiple pushFwd modules along a single f, you can do so at the expense of being explicit about which module you are pushing to - what else could you expect in this case!

[Devlin-Mallory]

I think this works quite well! I am happy not overloading 'pushFwd' and having 'pushforward' as the name for moving elements around. I like having the option to have multiple push forwards around and being able to specify which you intend!

[joel-dodge]

I'll update the PR and add documentation!

[Devlin-Mallory]

I can't quite find the line causing this problem, but I think there's a small issue arising from the caching of pushFwd, where running it again causes an error rather than returning the cached value:

f = id_S
pushFwd f -- works great
pushFwd f -- complains

[Devlin-Mallory]

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.

[joel-dodge]

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.

[Devlin-Mallory]

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.

[joel-dodge]

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?

[joel-dodge]

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!)

[joel-dodge]

I can't quite find the line causing this problem, but I think there's a small issue arising from the caching of pushFwd, where running it again causes an error rather than returning the cached value:

f = id_S
pushFwd f -- works great
pushFwd f -- complains

Excellent thank you! I also see this issue but the patch I have to update pushforward semantics fixes it. Let me push it.

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.

[joel-dodge]

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.

[mahrud]

You should keep them at the end of the package file.

[joel-dodge]

OK I will put them back at the bottom of the package file. Are the commands here executed at some point?

[joel-dodge]

Done!

[joel-dodge]

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.

[joel-dodge]

@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.

[Devlin-Mallory]

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).

[joel-dodge]

@Devlin-Mallory - Added a fix for the bug that you pointed out. I will comment where the fix was.
Build failures produced a nice bug coming from the use of PushForward in the Pullback package - now fixed as well along with a few other things.

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!

• @mahrud suggestion to rename NoPrune to MinimalGenerators and invert the default behavior seems very good to me. All it would take is an agreement from someone that this seems nice and I'll add it.
• Currently this does not support pushFwd of a module over an exterior algebra along a map that is not the inclusion of the coefficient ring. This is due to an issue computing the kernel in makeModule. I have a fix for this on my local branch that I'd like to include as well unless you request that it go into a separate PR.

[Devlin-Mallory]

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.)

[Devlin-Mallory]

Ah, very interesting! It looks like f and g really do give that M2 and M2' are isomorphic. Goes to show how hard it is to see if two things are isomorphic outside of the graded case. In that case, mathematically everything seems on the level, and I certainly don't see a reason to prefer M2 over M2' except that it happens to work better for our test. I think probably @mahrud and I should drill down into why summands works so much better for one presentation than the other; I'm not sure what best practices are in the meantime - you could comment out the test in this pull request? or we could make a separate PR disabling it for now? @mahrud what do you think?

[joel-dodge]

I'd suggest that if I comment out the test in the same commit that changes the pushFwd behavior the papertrail will be more obvious for where to look to understand what is going on with summands and eventually allow re-enabling the test.

[d-torrance]

FYI -- no-check-flag as a comment in the test will skip it without having to comment it out entirely

[joel-dodge]

Updating the PR description to match the current content of this PR so it is more useful. I am posting the previous desc here since it contained some useful content and examples and idk how easy it is to find / view PR description history in github:

This PR includes changes to the PushForward package to fix some buggy behavior and support a broader class of ring maps. Itemized changes include:

• handles pure SkewSymmetric polynomial rings
• computes relations better in towers
• Support case when R is not free over its coefficientRIng by including relations from the structure map.
• when lifting the accessor maps allow to fail over as in the inhomogeneous case when lifting fails. When the gradings of R and S interact this can happen.
• includes test cases to cover all of these. Add comments to all test cases in this file for easier matching with test runner output.

These are accomplished primarily by making modifications to the isModuleFinite and makeModule methods. 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:

• Investigate support for some helper maps related to pushforwards:
  • support accessor functions for pushFwd(Module) as in TODOs in PushForward.m2?
  • support a structure map S → Hom_R(pushFwd M, pushFwd M) exhibiting that the push forward came from an S-module?
• 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 already exists :D
• pushFwd of f: R → S is not implemented when S has skew commutative variables.

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:

i3 : S = ZZ/101[a..b, SkewCommutative => true]
o3 = S
o3 : PolynomialRing, 2 skew commutative variable(s)

i4 : isModuleFinite S
o4 = true

Towers of rings handled now. Previously last line would give false:

i9 : R = ZZ/101[a, b]
o9 = R
o9 : PolynomialRing

i10 : S = R/a^2
o10 = S
o10 : QuotientRing

i11 : T = S/b^2
o11 = T
o11 : QuotientRing

i12 : isModuleFinite T
o12 = true

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:

i13 : R = ZZ/101[a]/a^3
o13 = R
o13 : QuotientRing

i14 : S = R[b,c]
o14 = S
o14 : PolynomialRing

i15 : T = S / ideal {a^2, b^2, c^2}
o15 = T
o15 : QuotientRing

i16 : isModuleFinite T
o16 = true

i18 : (pushFwd T)#0
o18 = cokernel | a2 0  0  0  |
               | 0  a2 0  0  |
               | 0  0  a2 0  |
               | 0  0  0  a2 |

                             4
o18 : R-module, quotient of R

[mahrud]

I just realized a big flaw in this implementation, which is that it constructs a new ring every time you call isSkewAffineRing. I think instead when constructing skew commutative polynomial rings, R.isSkewAffineRing = true should be cached.

[joel-dodge]

Great catch I will change this!

[mahrud]

If you were making more changes to the documentation, I would say move this to the Contributors section.

[mahrud]

The trouble with global caching is that it can't be garbage collected and thus blows up the memory. This is why packages aren't supposed to use memoize, and as far as I can tell this essentially acts like memoize.

You should instead cache like:

pushAuxHgs RingMap := f -> f.cache.pushAuxHgs ??= (

[joel-dodge]

This makes perfect sense thank you.

[mahrud]

I believe annihilator is already cached, as is the quotient of a ring by an ideal. Is there an instance where it's not being cached correctly?

[joel-dodge]

I encountered an issue that had the flavor of this example:

R = ZZ/5[x];
(R / ideal x) === (R / ideal x); -- gives false

I will have to sniff around to see if I think that this really occurs in this branch though. If I can find a clear reason why it is needed, I'll share it otherwise I'll remove the caching. It may have only arisen in an exploration branch I had that did some more refactoring. Sorry for my confusion.

[mahrud]

New ideal forgets the grobner basis for the old one.

i1 : R = ZZ/5[x];

i2 : I = ideal x;

o2 : Ideal of R

i3 : R / I === R / I

o3 = true

[joel-dodge]

If there is a case in the current code where this caching of the annihilators was doing something then I have lost track of it. This is not relevant in any of the unit test cases. I am removing it!

[joel-dodge]

@Devlin-Mallory @mahrud - just checking in to see what is the current state of the review process for this PR. From your points of view what are the next steps here?

[joel-dodge]

Just added two new commits:

• I think I understand macaulay2 caching idioms better now so I revamped the way that caching is done in this PR.
• Based on my experience working with this package on further developments i have locally, I think that using the implicit inclusion of the coefficient ring for the pushforward method produces unpleasant sharp edges that are easy to run into. I have removed it in a commit here with the thinking that it is easier to add later if the need arises than it is to remove it once it is part of the API. There was some discussion in the comments of this PR about exactly this point much earlier.

It would be easy to revert either of these changes if discussion leads us to think that would be best.

[Devlin-Mallory]

@Devlin-Mallory @mahrud - just checking in to see what is the current state of the review process for this PR. From your points of view what are the next steps here?

Apologies, I've been slow in getting to this but am hoping to finish looking it over in the next few days! I'm probably the limiting factor here, rather than any next steps you need to do.

[joel-dodge]

Apologies, I've been slow in getting to this but am hoping to finish looking it over in the next few days! I'm probably the limiting factor here, rather than any next steps you need to do.

Makes sense ty for the update!

[joel-dodge]

BTW I suspect that at least some of the bug examples here can be moved to test-cases if / when #4435 is merged.

Specifically, the error that is raised in many (maybe all but I did not carefully check every one) of these bugs is exactly the one raised by the core pushforward code when the degree lift is funky.

This is not really germane to this review but is part of general context about this package that I know reviewers are interested in!

[Devlin-Mallory]

Might be worth adding a line of history attributing the significant refactoring and additions in this PR to yourself!

[joel-dodge]

ty for the suggestion. done!

[Devlin-Mallory]

From my point of view, everything in the pull request looks good and ready to merge - I've been experimenting with it a bunch and everything seems to work well in the contexts I'm working in. Speed seems essentially comparable to before on the experiments I've tried (sometimes slightly slower, sometimes slightly faster). I don't know the skew commutative stuff story well, so haven't experimented much with it, but the code looks good and it's well covered by tests.

[joel-dodge]

Back in April when I asked about SOP for macaulay2 PRs mahrud said:

Usually after the review we ping the package authors to make sure they agree with the changes before merging.

Is it time to ping any package authors to get their review now?