Some contributions to the PushForward package

[joel-dodge]

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
• kernel 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]

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!