Some improvements on the modules #3041

HechtiDerLachs · 2023-11-23T17:39:52Z

@jankoboehm : I wanted to look a bit into the morphisms of modules for the reasons we talked about today. But before I could actually start doing that, I needed to split that gigantic file into more digestable parts. For me personally this was so that syntax highlighting would not crash all the time. But also in general, I think, it makes sense to split this into thematically ordered smaller chunks.

Do you agree?

Edit: splitting now takes place in #3042 . This PR will eventually be rebased. Until then check out the single last commit to see the changes for the improvements only.

codecov · 2023-11-23T19:44:26Z

Codecov Report

Merging #3041 (0df44a6) into master (a60a7c0) will decrease coverage by 0.01%.
Report is 3 commits behind head on master.
The diff coverage is 85.48%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3041      +/-   ##
==========================================
- Coverage   80.43%   80.43%   -0.01%     
==========================================
  Files         520      520              
  Lines       70074    70175     +101     
==========================================
+ Hits        56365    56446      +81     
- Misses      13709    13729      +20

Files	Coverage Δ
src/Modules/UngradedModules/DirectSum.jl	`83.84% <100.00%> (ø)`
src/Modules/UngradedModules/FreeModElem.jl	`96.00% <100.00%> (+0.39%)`	⬆️
src/Modules/UngradedModules/Presentation.jl	`97.67% <100.00%> (ø)`
...c/Modules/UngradedModules/SubModuleOfFreeModule.jl	`71.21% <100.00%> (+0.14%)`	⬆️
src/Modules/UngradedModules/Tensor.jl	`93.58% <100.00%> (ø)`
src/Modules/UngradedModules/Methods.jl	`85.30% <66.66%> (ø)`
src/Modules/ModulesGraded.jl	`81.25% <66.66%> (ø)`
src/Modules/UngradedModules/FreeModuleHom.jl	`85.60% <83.33%> (-0.11%)`	⬇️
src/Modules/UngradedModules/SubquoModuleElem.jl	`85.27% <94.00%> (+2.30%)`	⬆️
src/Modules/ModuleTypes.jl	`76.92% <84.84%> (+0.14%)`	⬆️
... and 1 more

... and 5 files with indirect coverage changes

HechtiDerLachs · 2023-11-23T21:02:06Z

I tried to improve the code a bit, especially regarding the speed of evaluation of mappings. So far running the same test-suite for modules gives

before:

 45.207242 seconds (433.82 M allocations: 20.793 GiB, 12.69% gc time, 17.17% compilation time)
 45.368074 seconds (433.65 M allocations: 20.792 GiB, 13.04% gc time, 17.48% compilation time)

after

 37.961641 seconds (265.70 M allocations: 12.739 GiB, 10.21% gc time, 21.95% compilation time)
 38.276709 seconds (266.08 M allocations: 12.747 GiB, 7.61% gc time, 22.06% compilation time)
 37.249246 seconds (265.81 M allocations: 12.744 GiB, 7.62% gc time, 21.19% compilation time)

So especially in terms of memory consumption, I think this is useful and should be merged one way or the other.

Edit: The main upshots were:

SubquoModuleElems can now store either their coordinates or their representative. Thus, access via the fields a.coeffs and a.repres is now forbidden and the getters coordinates(a) and repres(a) should be used instead.
Addition and multiplication of SubQuoElems uses whatever of the two is there, but doesn't fill out the other side all the time.
SubquoElems now have a field is_reduced similar to the elements in quotient rings.
Maps store a list of the images of the generators and this can be called via images_of_generators(f) (and not through creation of the inclusion of the image and accessing the generators of that submodule as before...)
Evaluation of maps use the images of generators and coordinates of the element to be mapped.

thofma · 2023-11-23T21:15:27Z

Why not separate the code reorganization from the improvements PR wise? The spitting should definitely be happening.

HechtiDerLachs · 2023-11-24T09:47:26Z

I ran some more tests locally. Compared to current master I now get the following timings:

before:

46.521581 seconds (434.29 M allocations: 20.809 GiB, 12.73% gc time, 16.09% compilation time)
 47.341869 seconds (433.96 M allocations: 20.798 GiB, 9.42% gc time, 16.86% compilation time)

after:

 39.683286 seconds (273.27 M allocations: 12.962 GiB, 10.36% gc time, 21.05% compilation time)
 38.410556 seconds (273.16 M allocations: 12.958 GiB, 7.44% gc time, 21.16% compilation time)
 39.105477 seconds (273.33 M allocations: 12.962 GiB, 7.44% gc time, 20.39% compilation time)

The difference to the above runs is due to some tests being disabled (on both sides) before. These runs here really take the whole test suite for the modules.

HechtiDerLachs · 2023-11-24T09:49:22Z

@jankoboehm : I double checked again. There is no internal constructor for neither FreeModuleHom nor SubQuoHom which does not take the images of the generators as input. So computing and caching them only on request has never been an option.

Edit: Besides: Now that the SubquoModuleElems are allowed to also store the information on their coordinates only, this should be just as good as what you had in mind: For every image of a generator store the coordinates in the generators of the codomain. Just make sure that the list of images you are passing to the constructor do not contain the fields on their representatives.

HechtiDerLachs · 2023-11-24T13:46:13Z

Timings now go down to

 38.644607 seconds (268.27 M allocations: 12.779 GiB, 7.30% gc time, 20.29% compilation time)
 38.324128 seconds (268.55 M allocations: 12.780 GiB, 7.56% gc time, 21.69% compilation time)

No major improvement anymore. But still something. I think I can stop here.

Could maybe someone confirm the changes with the hashing?

HechtiDerLachs · 2023-11-27T17:28:14Z

Can this be merged, then?

fingolfin · 2023-11-29T10:59:20Z

src/Modules/UngradedModules/FreeModElem.jl

  b = 0xaa2ba4a32dd0b431 % UInt
  h = hash(typeof(a), h)
  h = hash(parent(a), h)
  h = hash(coordinates(a), h)
  return xor(h, b)
 end

+function hash(a::AbstractFreeModElem{<:MPolyQuoRingElem}, h::UInt)
+  simplify!(a)


We just discussed how this call resp. this hash method is somewhat dangerous:

it assumes that simplify!(a) modifies a in-place and also "remembers" that it is simplified (so it is fast "amortized", i.e. you only compute a GB for it once)

it must produce a unique normal form -- but we don't really have a specification for simplify!. We really need one otherwise we are building on quicksand

perhaps it would be better to just do error("not supported") here and handle using these in dicts differently??

however the old hash(a::AbstractFreeModElem, h::UInt) method just was wrong...

fingolfin · 2023-11-29T11:01:09Z

src/Modules/UngradedModules/FreeModElem.jl

+  return xor(h, b)
+end
+
+simplify(a::AbstractFreeModElem{<:MPolyQuoRingElem}) = return parent(a)(map_entries(simplify, coordinates(a)))


Suggested change

simplify(a::AbstractFreeModElem{<:MPolyQuoRingElem}) = return parent(a)(map_entries(simplify, coordinates(a)))

simplify(a::AbstractFreeModElem{<:MPolyQuoRingElem}) = parent(a)(map_entries(simplify, coordinates(a)))

fingolfin · 2023-11-29T11:06:15Z

src/Modules/UngradedModules/FreeModuleHom.jl

@@ -553,9 +559,9 @@ Homogeneous module homomorphism)
 ```
 """
 function image(h::FreeModuleHom)
-  si = [x for x = map(h, basis(domain(h))) if !iszero(x)]
+  si = filter(x->!iszero(x), images_of_generators(h))


Suggested change

si = filter(x->!iszero(x), images_of_generators(h))

si = filter(!iszero, images_of_generators(h))

fingolfin

While there issues with the hash method, the existing method is just as broken, so this doesn't really make it worse... We could merge this PR, and open an issue to remind us to resolve the hash function (and also to specify simplify!)

fingolfin · 2023-11-29T14:08:31Z

src/Modules/UngradedModules/FreeModElem.jl

-function hash(a::AbstractFreeModElem, h::UInt)
+# A special method for the case where we can safely assume 
+# that the coordinates of elements allow hashing.
+function hash(a::AbstractFreeModElem{<:MPolyElem{<:FieldElem}}, h::UInt)


Note that a field element does not necessarily have correct hashing implemented either -- though as you correctly just pointed out to me when we talked, such fields probably won't be supported by Singular either.

But on the long run we probably want a bunch of type traits to model whether hashing, normal forms, etc. are supported:

can_hash(::Type) = false can_compute_normal_form(::Type) = false can_hash(::Type{<:MPolyElem{T}}) where T = can_hash(T) can_compute_normal_form(::Type{<:MPolyElem{T}}) where T = can_compute_normal_form(T)

etc.

(Perhaps should turn this into an issue?)

src/Modules/UngradedModules/FreeModuleHom.jl

HechtiDerLachs · 2023-11-29T15:35:16Z

Setting the generic hash function to error was not an option in the end. Even the unique function uses hashes and I think we can not pull all of such uses out of the code for the moment.

HechtiDerLachs changed the title ~~Split UngradedModules.jl into separate files~~ Split UngradedModules.jl into separate files and some improvements Nov 23, 2023

HechtiDerLachs requested a review from jankoboehm November 23, 2023 21:02

HechtiDerLachs changed the title ~~Split UngradedModules.jl into separate files and some improvements~~ Some improvements on the modules Nov 23, 2023

HechtiDerLachs mentioned this pull request Nov 23, 2023

Module migration #3042

Merged

HechtiDerLachs force-pushed the module_map_optimizations branch from 6431e23 to 29ddc51 Compare November 24, 2023 09:44

HechtiDerLachs force-pushed the module_map_optimizations branch from 5c2553e to 4270dbe Compare November 24, 2023 16:16

HechtiDerLachs and others added 7 commits November 24, 2023 20:49

Some improvements on modules and their morphisms.

ad8db16

Some further tweaks on module maps.

12994d5

Some fixes.

f78aeb3

Some more improvements.

51e2a73

Some more tweaks.

065f303

Reapply switch to getter after rebase [no ci].

9a1597f

Add some type stability to allow for zero morphisms.

81a4988

HechtiDerLachs force-pushed the module_map_optimizations branch from 295679b to 81a4988 Compare November 24, 2023 19:50

HechtiDerLachs and others added 2 commits November 24, 2023 21:05

Fix tests.

7a81154

Fix doctests.

0df44a6

HechtiDerLachs mentioned this pull request Nov 24, 2023

Further functionality for hyper complexes #3021

Merged

fingolfin reviewed Nov 29, 2023

View reviewed changes

fingolfin approved these changes Nov 29, 2023

View reviewed changes

Amend changes from today's discussion.

b876d3d

fingolfin reviewed Nov 29, 2023

View reviewed changes

Set default hash function to error.

2dba828

fingolfin reviewed Nov 29, 2023

View reviewed changes

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

HechtiDerLachs and others added 2 commits November 29, 2023 15:14

More on hashes.

0a11318

Update src/Modules/UngradedModules/FreeModuleHom.jl

7496697

fingolfin enabled auto-merge (squash) November 29, 2023 14:15

Revert to a dummy hash function.

65a3fa9

HechtiDerLachs added 2 commits November 30, 2023 10:25

Fix some false flags.

9a4046f

Attempt to fix doctests.

db03eea

fingolfin merged commit 8e1880f into oscar-system:master Nov 30, 2023
14 of 19 checks passed

HechtiDerLachs deleted the module_map_optimizations branch November 30, 2023 15:29

HechtiDerLachs mentioned this pull request Jan 24, 2024

Fix sub and quo for modules #3237

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some improvements on the modules #3041

Some improvements on the modules #3041

HechtiDerLachs commented Nov 23, 2023 •

edited

Loading

codecov bot commented Nov 23, 2023 •

edited

Loading

HechtiDerLachs commented Nov 23, 2023 •

edited

Loading

thofma commented Nov 23, 2023

HechtiDerLachs commented Nov 24, 2023

HechtiDerLachs commented Nov 24, 2023 •

edited

Loading

HechtiDerLachs commented Nov 24, 2023

HechtiDerLachs commented Nov 27, 2023

fingolfin Nov 29, 2023

fingolfin Nov 29, 2023

fingolfin Nov 29, 2023

fingolfin left a comment

fingolfin Nov 29, 2023

HechtiDerLachs commented Nov 29, 2023

	simplify(a::AbstractFreeModElem{<:MPolyQuoRingElem}) = return parent(a)(map_entries(simplify, coordinates(a)))
	simplify(a::AbstractFreeModElem{<:MPolyQuoRingElem}) = parent(a)(map_entries(simplify, coordinates(a)))

	si = filter(x->!iszero(x), images_of_generators(h))
	si = filter(!iszero, images_of_generators(h))

Some improvements on the modules #3041

Some improvements on the modules #3041

Conversation

HechtiDerLachs commented Nov 23, 2023 • edited Loading

codecov bot commented Nov 23, 2023 • edited Loading

Codecov Report

HechtiDerLachs commented Nov 23, 2023 • edited Loading

thofma commented Nov 23, 2023

HechtiDerLachs commented Nov 24, 2023

HechtiDerLachs commented Nov 24, 2023 • edited Loading

HechtiDerLachs commented Nov 24, 2023

HechtiDerLachs commented Nov 27, 2023

fingolfin Nov 29, 2023

Choose a reason for hiding this comment

fingolfin Nov 29, 2023

Choose a reason for hiding this comment

fingolfin Nov 29, 2023

Choose a reason for hiding this comment

fingolfin left a comment

Choose a reason for hiding this comment

fingolfin Nov 29, 2023

Choose a reason for hiding this comment

HechtiDerLachs commented Nov 29, 2023

HechtiDerLachs commented Nov 23, 2023 •

edited

Loading

codecov bot commented Nov 23, 2023 •

edited

Loading

HechtiDerLachs commented Nov 23, 2023 •

edited

Loading

HechtiDerLachs commented Nov 24, 2023 •

edited

Loading