Adds free resolutions over quotient rings by using `Singular.sres` #4134

RafaelDavidMohr · 2024-09-23T14:40:20Z

Adds free resolutions over quotient rings by using Singular.sres (with fres currently not working for quotient rings in Singular).

Some Macaulay2 people told me that their users were confused by the fact that they could compute free resolutions over quotient rings without specifying a length, so I force the user to set a length for the free resolution when computing over quotient rings.

CC: @ederc

wdecker · 2024-09-24T09:49:45Z

Some Macaulay2 people told me that their users were confused by the fact that they could compute free resolutions over quotient rings without specifying a length, so I force the user to set a length for the free resolution when computing over quotient rings.

That is o.k. Just for the record: entering zero would also be o.k. since then Singular stops after number of variables +1 steps.

HechtiDerLachs · 2024-09-24T15:34:26Z

src/Modules/UngradedModules/ModuleGens.jl

-      push!(values, MPolyBuildCtx(base_ring(F)))
+      push!(values, zero(Rx))


Were these deliberately reverted because we do not have an MPolyBuildCtx over quotient rings? In that case, I suggest to refactor this part of the code via type dispatch. We talked about it during last week's workshop how to speed this up, eventually. Also, I think @Lax202 's work in progress on non-commutative groebner bases might touch this part again. So we should eventually decide how to split this up here.

Edit: This does not need to hold this PR back for now. But before anyone merges, we should open an issue to remind us to clean this bit up.

Follow up: I tried do speed up some of our computations yesterday and made a patch for this part here in #4147 . Would it be desirable to push it directly to this PR? If so, please let me know.

Thanks for taking a look and fixing this! Merging this in unison would be good for me.

HechtiDerLachs · 2024-09-24T15:35:36Z

Thanks for doing this! Actually, I have a big computation going on right now which could potentially benefit from this.

afkafkafk13 · 2024-09-25T05:14:24Z

src/Modules/UngradedModules/FreeResolutions.jl

+  if T <: MPolyQuoRingElem
+    !iszero(length) || error("Specify a length up to which a free resolution should be computed")
+  end
+


Singular's sres sets a default of nvars(basering) modules to be computed, if no length is specified. It might be better for compatibility to document this fallback instead of throwing an error.

Thank you for taking a look. Having this default nvars(basering) length is the same they had in Macaulay2, they told me that they removed this at some point for clarity for the users.

But if you prefer to have this default, I will of course make the appropriate changes.

wdecker · 2024-09-25T11:06:33Z

I think you can leave it as it is now.

…

Am 25.09.2024 um 12:58 schrieb Rafael Mohr ***@***.***>: @RafaelDavidMohr commented on this pull request. In src/Modules/UngradedModules/FreeResolutions.jl <#4134 (comment)>: > + if T <: MPolyQuoRingElem + !iszero(length) || error("Specify a length up to which a free resolution should be computed") + end + Thank you for taking a look. Having this default nvars(basering) length is the same they had in Macaulay2, they told me that they removed this at some point for clarity for the users. But if you prefer to have this default, I will of course make the appropriate changes. — Reply to this email directly, view it on GitHub <#4134 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AASSXET5HLNPO6YQTY3ZSULZYKJNTAVCNFSM6AAAAABOWHW3DKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMRXHEYTCOBVGE>. You are receiving this because you commented.

HechtiDerLachs · 2024-09-25T11:09:37Z

src/Modules/UngradedModules/ModuleGens.jl

+# if isone(length(s)) # TODO: length doesn't work!
+#   (i, e, c) = first(s)
+#   push_term!(ctx, R(c), e)
+#   return FreeModElem(sparse_row(Qx, [(i, finish(ctx))]))
+# end


Does anyone know what is the correct way to check whether a Singular.svector has only one term? length throws an error, even though it should be part of the iterator interface.

length(p::svector) = Int(libSingular.pLength(p.ptr))

should do it

see oscar-system/Singular.jl#833

Update: @hannes14 's PR on Singular.jl just got merged. Do we have to wait for a release there? Or can we remove the method here already?

ping @lgoettgens

You will need to wait for a new release of Singular.jl, and then adapt the compat entry in Oscar's Project.toml to this new version number

lgoettgens · 2024-09-25T11:48:35Z

src/Modules/UngradedModules/ModuleGens.jl

@@ -283,18 +283,21 @@ function (F::FreeMod{<:MPolyRingElem})(s::Singular.svector)
  return FreeModElem(sparse_row(Rx, [(i, finish(ctx)) for (i, ctx) in cache]), F)
 end

+# TODO: move this eventually
+length(s::Singular.svector) = Int(Singular.libSingular.pLength(s.ptr))


Please move this to Singular.jl before merging this PR. Sorting this out afterwards is more of a hassle than doing it right the first time

src/Modules/UngradedModules/ModuleGens.jl

HechtiDerLachs · 2024-09-25T13:04:05Z

My changes seem to work for now. It remains to clean up some of the tests.

test/Modules/MPolyQuo.jl

Co-authored-by: Matthias Zach <[email protected]>

RafaelDavidMohr · 2024-09-26T08:43:33Z

Thanks @HechtiDerLachs for fixing the last tests. Is there any interest in keeping the option to compute free resolutions over quotients by iterative kernel computations in addition to using sres?

HechtiDerLachs · 2024-09-26T08:49:19Z

Is there any interest in keeping the option to compute free resolutions over quotients by iterative kernel computations in addition to using sres?

I think it's always good to have different options for the user and a reasonable default. Eventually it would be nice to have the computation lazy, but using sres. But this is probably to be decided elsewhere and not on this PR. If the tests pass, then from my side I'm ok with the iterative kernel computation being disabled for the moment. We still have the code to put it back in and I am probably using the hypercomplexes, anyway.

@jankoboehm : Any opinion on this matter?

HechtiDerLachs · 2024-09-26T09:22:03Z

Some timings for @time Oscar.test_module("test/Modules/", new=false)

On current master:
37.206091 seconds (185.12 M allocations: 9.750 GiB, 24.01% gc time, 20.60% compilation time)
37.702440 seconds (184.58 M allocations: 9.738 GiB, 22.81% gc time, 21.73% compilation time)

On the head of this PR:
37.933323 seconds (189.73 M allocations: 9.926 GiB, 23.15% gc time, 23.39% compilation time)
38.951809 seconds (190.67 M allocations: 9.939 GiB, 22.77% gc time, 19.89% compilation time)

On 3d35d0a, i.e. not using MPolyBuildCtx for the conversion
42.981092 seconds (215.30 M allocations: 10.906 GiB, 21.85% gc time, 17.19% compilation time)
42.981092 seconds (215.30 M allocations: 10.906 GiB, 21.85% gc time, 17.19% compilation time)
41.036220 seconds (214.36 M allocations: 10.897 GiB, 22.19% gc time, 18.05% compilation time)
43.827199 seconds (215.27 M allocations: 10.907 GiB, 21.35% gc time, 20.50% compilation time)

I had to disable two lines in the tests for the modules over MPolyQuoRing for the latter.

codecov · 2024-09-26T09:52:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.68%. Comparing base (55ea957) to head (de3ec65).
Report is 8 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4134      +/-   ##
==========================================
+ Coverage   84.65%   84.68%   +0.02%     
==========================================
  Files         626      627       +1     
  Lines       84313    84324      +11     
==========================================
+ Hits        71377    71407      +30     
+ Misses      12936    12917      -19

Files with missing lines	Coverage Δ
src/Modules/UngradedModules/FreeResolutions.jl	`81.30% <100.00%> (-8.03%)`	⬇️
src/Modules/UngradedModules/ModuleGens.jl	`75.00% <100.00%> (+2.17%)`	⬆️
src/Modules/UngradedModules/Presentation.jl	`94.02% <100.00%> (+0.04%)`	⬆️

... and 18 files with indirect coverage changes

HechtiDerLachs · 2024-09-26T11:56:00Z

You will need to wait for a new release of Singular.jl, and then adapt the compat entry in Oscar's Project.toml to this new version number

This is still pending. Other than that, this seems good to go, I think.

lgoettgens · 2024-09-27T15:50:23Z

You will need to wait for a new release of Singular.jl, and then adapt the compat entry in Oscar's Project.toml to this new version number

This is still pending. Other than that, this seems good to go, I think.

Singular.jl v0.23.8 is now available

src/Modules/UngradedModules/ModuleGens.jl

lgoettgens · 2024-09-27T16:05:17Z

I added the compat bump myself. No objections from me anymore

Rafael Mohr added 7 commits June 6, 2024 17:00

adapt module gens to quotient rings

6b9dcc6

adjusts signatures of prune_with_map and _presentation_minimal

f8a555b

Merge branch 'master' into rm/fres-quotient-ring

3d35d0a

compute free resolutions over quotients with sres

6925b56

updates docu

f47f456

adds test

14c7ab3

fix tests

452dd02

wdecker approved these changes Sep 24, 2024

View reviewed changes

fix another test

8588359

HechtiDerLachs reviewed Sep 24, 2024

View reviewed changes

HechtiDerLachs mentioned this pull request Sep 24, 2024

Attempt to clear up mpolyquo modules #4147

Draft

afkafkafk13 reviewed Sep 25, 2024

View reviewed changes

Add dispatch for singular-to-oscar conversion [no ci].

71cbe8c

HechtiDerLachs reviewed Sep 25, 2024

View reviewed changes

Enable length shortcut.

5c2a895

lgoettgens reviewed Sep 25, 2024

View reviewed changes

HechtiDerLachs reviewed Sep 25, 2024

View reviewed changes

src/Modules/UngradedModules/ModuleGens.jl Outdated Show resolved Hide resolved

HechtiDerLachs and others added 2 commits September 25, 2024 14:00

Update src/Modules/UngradedModules/ModuleGens.jl

bb81af4

Fix tests.

16985d5

fix test

846a464

HechtiDerLachs reviewed Sep 26, 2024

View reviewed changes

test/Modules/MPolyQuo.jl Outdated Show resolved Hide resolved

HechtiDerLachs reviewed Sep 26, 2024

View reviewed changes

test/Modules/MPolyQuo.jl Outdated Show resolved Hide resolved

RafaelDavidMohr and others added 2 commits September 26, 2024 10:41

fix test

11bf9f4

Co-authored-by: Matthias Zach <[email protected]>

fix test

de3ec65

Co-authored-by: Matthias Zach <[email protected]>

RafaelDavidMohr marked this pull request as ready for review September 26, 2024 08:43

lgoettgens reviewed Sep 27, 2024

View reviewed changes

src/Modules/UngradedModules/ModuleGens.jl Outdated Show resolved Hide resolved

lgoettgens added 2 commits September 27, 2024 17:59

Bump Singular compat

3db4e44

Update src/Modules/UngradedModules/ModuleGens.jl

64f6b44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds free resolutions over quotient rings by using `Singular.sres` #4134

Adds free resolutions over quotient rings by using `Singular.sres` #4134

RafaelDavidMohr commented Sep 23, 2024

wdecker commented Sep 24, 2024

HechtiDerLachs Sep 24, 2024 •

edited

Loading

HechtiDerLachs Sep 25, 2024

RafaelDavidMohr Sep 25, 2024

HechtiDerLachs commented Sep 24, 2024

afkafkafk13 Sep 25, 2024

RafaelDavidMohr Sep 25, 2024

wdecker commented Sep 25, 2024 via email

HechtiDerLachs Sep 25, 2024

hannes14 Sep 25, 2024

hannes14 Sep 25, 2024

HechtiDerLachs Sep 26, 2024

lgoettgens Sep 26, 2024

lgoettgens Sep 25, 2024

HechtiDerLachs commented Sep 25, 2024

RafaelDavidMohr commented Sep 26, 2024

HechtiDerLachs commented Sep 26, 2024

HechtiDerLachs commented Sep 26, 2024 •

edited

Loading

codecov bot commented Sep 26, 2024

HechtiDerLachs commented Sep 26, 2024

lgoettgens commented Sep 27, 2024

lgoettgens commented Sep 27, 2024

		push!(values, MPolyBuildCtx(base_ring(F)))
		push!(values, zero(Rx))

Adds free resolutions over quotient rings by using Singular.sres #4134

Are you sure you want to change the base?

Adds free resolutions over quotient rings by using Singular.sres #4134

Conversation

RafaelDavidMohr commented Sep 23, 2024

wdecker commented Sep 24, 2024

HechtiDerLachs Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HechtiDerLachs commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wdecker commented Sep 25, 2024 via email

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

HechtiDerLachs commented Sep 25, 2024

RafaelDavidMohr commented Sep 26, 2024

HechtiDerLachs commented Sep 26, 2024

HechtiDerLachs commented Sep 26, 2024 • edited Loading

codecov bot commented Sep 26, 2024

Codecov Report

HechtiDerLachs commented Sep 26, 2024

lgoettgens commented Sep 27, 2024

lgoettgens commented Sep 27, 2024

Adds free resolutions over quotient rings by using `Singular.sres` #4134

Adds free resolutions over quotient rings by using `Singular.sres` #4134

HechtiDerLachs Sep 24, 2024 •

edited

Loading

HechtiDerLachs commented Sep 26, 2024 •

edited

Loading