Migrate `MapFromFunc` from Hecke->AbstractAlgebra by mjrodgers · Pull Request #2443 · Nemocas/AbstractAlgebra.jl

mjrodgers · 2026-06-29T12:42:25Z

As discussed in 6043, there is some motivation to move MapFromFunc from Hecke into AbstractAlgebra, at least partially so that it can be used in GAP.jl/NemoExt (we hope to migrate some of the iso_oscar_gap/iso_gap_oscar functionality to there.

To facilitate this, the following things have been moved:

the abstract type HeckeMap,
MapFromFunc
MapHeader
MapCache
and some related/supporting functions.

We will also move some related tests, and rewrite some code using AbstractAlgebra.map_from_func (which creates a Generic.FunctionalMap) to use MapFromFunc instead.

mjrodgers · 2026-06-29T14:20:32Z

Some weird things I am noticing with this:

To some extent, the abstract type HeckeMap and the concrete type MapFromFunc are replacements for the abstract type FunctionalMap and corresponding concrete type Generic.FunctionalMap, although the correspondence is not one-to-one.

The concrete Generic.FunctionalMap is very rarely used, I think only by the old map_from_func which we are in the process of deprecating in favor of MapFromFunc; but it seems like there are some (maybe minor?) limitations, one that I've come across is that if we compose two Generic.FunctionalMap the result still retains the knowledge that it is a functional map (Generic.FunctionalCompositeMap <: FunctionalMap) whereas if we compose two MapFromFuncs then we just get a Generic.CompositeMap.

Anyway, I don't know if this is a big deal at all, but I'm mainly wondering if it would be reasonable or useful to somehow merge the functionalities of HeckeMap and FunctionalMap, or if this would be really really really breaking.

codecov · 2026-06-29T14:25:41Z

Codecov Report

❌ Patch coverage is 88.65979% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.13%. Comparing base (e440963) to head (c5bbbe1).
⚠️ Report is 14 commits behind head on master.

Files with missing lines	Patch %	Lines
src/MapTypes.jl	87.67%	9 Missing ⚠️
src/Map.jl	90.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2443      +/-   ##
==========================================
- Coverage   88.14%   88.13%   -0.02%     
==========================================
  Files         129      130       +1     
  Lines       33017    33113      +96     
==========================================
+ Hits        29104    29185      +81     
- Misses       3913     3928      +15

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

thofma · 2026-06-29T22:23:36Z

I think it is fine to get a CompositeMap.

Should we also move map_from_func here?

mjrodgers · 2026-06-30T06:27:38Z

I think it is fine to get a CompositeMap.

Should we also move map_from_func here?

I think maybe we should replace the current AA map_from_func with the new version that we use in OSCAR (and my Hecke pull request implements it there, also) - this has a different argument order and return type so is breaking. So first I need to make a pull request to Nemo, that is one of the few places where this old version is being used. (I'll work on this soon).

thofma · 2026-06-30T07:13:39Z

I think we are breaking anyway, so let's try to do all the breaking stuff at once.

mjrodgers · 2026-06-30T07:15:37Z

Sounds good, I'll get the modifications submitted to Nemo and then fix this up to move fully to the new map_from_func

mjrodgers · 2026-06-30T11:09:51Z

Some things to consider:

There is some documentation related to FunctionalMap (the generic type) in docs/src/functional_map.md which mentions that we can create a concrete Generic functional map using map_from_func; but now this method returns a MapFromFunc, which is not a subtype of FunctionalMap. I'm a little uncertain how to update this.
There are now two implementations of MapCache (one is Generic.MapCache and the other is newly imported from Hecke), I'm not sure if this creates issues or not. They are slightly different in the number of parameters they take, and the Hecke one also can cache preimages.

The Hecke maps use a MapHeader which has a cache::MapCache field, so maybe 2. isn't a problem; HeckeMap uses the Hecke MapCache, and all other maps (if cached) use Generic.MapCache.

mjrodgers · 2026-06-30T15:46:07Z

I think it is fine to get a CompositeMap.

Should we also move map_from_func here?

I'm testing this with having HeckeMap <: FunctionalMap, there are some weird test failures resulting from composing two MapFromFuncs and getting a Generic.CompositeMap, which the generic image_fn method for functional maps doesn't apply to. Maybe this is a design decision that is overall bad for reasons I am overlooking, but I think it makes sense, the only requirement to be a FunctionalMap is to have an image_fn method which all HeckeMaps have.

EDIT: Having trouble getting this to work, I have Map(MapFromFunc) <: Map(FunctionalMap), but when I try to compose two MapFromFuncs julia uses the generic compose(f::Map, g::Map) instead of compose(f::Map(FunctionalMap){D, U}, g::Map(FunctionalMap){U, C}) like I want...

EDIT2: ooooooh since this code is in Generic this signature for compose only applies to the Generic.FunctionalMap type. Ok, I need to think about what to do here, is it a problem that Generic.CompositeMap does not support image_fn? (If not I can just remove the offending tests, I don't know.) If this is something we need, then I could either make a compose method for two MapFromFunc that either returns a FunctionalCompositeMap, or else replace with a new FromFuncCompositeMap (naming TBD) that would still be a HeckeMap.

fingolfin · 2026-07-08T12:28:46Z

-function map_from_func(image_fn::Function, domain, codomain)
-   return Generic.FunctionalMap(domain, codomain, image_fn)
+map_from_func(D, C, image_fn) = MapFromFunc(D, C, image_fn)
+map_from_func(D, C, image_fn, inverse_fn) = MapFromFunc(D, C, image_fn, inverse_fn)


How about we provide this for backwards compatibility:

Suggested change

map_from_func(D, C, image_fn, inverse_fn) = MapFromFunc(D, C, image_fn, inverse_fn)

@deprecated map_from_func(image_fn::Function, D, C) = MapFromFunc(D, C, image_fn)

(in the appropriate file)

fingolfin · 2026-07-08T12:30:28Z

+#
+###############################################################################
+
+mutable struct MapCache{D, C, De, Ce}


It is unfortunate that we also have Generic.MapCache; the similar names may lead to confusion. I actually think the name Generic.MapCache is "wrong" by our modern convention (it should be something like Generic.CacheMap or Generic.CachedMap), but it probably isn't worth the bother to update it -- rather I'd consider ditching it in the not so distant future (but not as part of this PR).

Perhaps we can deprecate the method related to Generic.MapCache sooner, though (so enable_cache, or cached(::Map) etc.

Since we are adding MapFromFunc here, I'd also be OK with calling this Generic.MapWithCache... as long as the name clearly conveys "this is a type of Map". (But of course I'd still rather prefer removing it ... ;-) )

fingolfin · 2026-07-08T12:33:08Z

+  old_im
+  old_pr
+
+  function MapCache(dom::D, cod::C, ::Type{De}, ::Type{Ce}, lim::Int=100) where {D, C, De, Ce}


In Hecke there are function allow_cache! and stop_cache! for all maps with a map header. We could also migrate them here.

fingolfin · 2026-07-08T12:33:58Z

+    z = new{D, C}()
+    z.domain = domain
+    z.codomain = codomain
+    return z


Suggested change

z = new{D, C}()

z.domain = domain

z.codomain = codomain

return z

return new{D, C}(domain, codomain)

fingolfin · 2026-07-08T12:34:16Z

+    z = new{D, C}()
+    z.domain = domain
+    z.codomain = codomain
+    z.image = image
+    return z


Suggested change

z = new{D, C}()

z.domain = domain

z.codomain = codomain

z.image = image

return z

return new{D, C}(domain, codomain, image)

fingolfin · 2026-07-08T12:34:39Z

+    z = new{D, C}()
+    z.domain = domain
+    z.codomain = codomain
+    z.image = image
+    z.preimage = preimage
+    return z


Suggested change

z = new{D, C}()

z.domain = domain

z.codomain = codomain

z.image = image

z.preimage = preimage

return z

return new{D, C}(domain, codomain, image, preimage)

fingolfin · 2026-07-08T12:35:15Z

+  image
+  preimage


Suggested change

image

preimage

image # function (or other kind of object) used to actually compute images

preimage # function (or other kind of object) used to actually compute preimages

fingolfin · 2026-07-08T12:47:52Z


 abstract type IdentityMap <: SetMap end

+abstract type HeckeMap <: FunctionalMap end


Suggested change

abstract type HeckeMap <: FunctionalMap end

abstract type HeckeMap <: SetMap end

How about we rename this to MapWithHeader or so?

@thofma opinion?

To make the transition on the Hecke side easier, the Hecke PR thofma/Hecke.jl#2290 could define an alias const HeckeMap = AbstractAlgbra.MapWithHeader ...?

fingolfin · 2026-07-08T12:51:54Z

+julia> g = map_from_func(ZZ, QQ, x -> QQ(x));

 julia> h = compose(f, g)
-Functional composite map


We should just delete FunctionalCompositeMap -- and eventually also Generic.FunctionalMap

To get rid of Generic.FunctionalMap we also need to change map_with_preimage_from_func etc. to call map_from_func

fingolfin · 2026-07-08T13:04:23Z


 abstract type Map{D, C, S, T} <: SetElem end

 abstract type SetMap end


Suggested change

#

# The following types are only used as type parameters for `Map` subtypes

#

abstract type SetMap end

mjrodgers added 2 commits June 26, 2026 13:34

initial attempt to move MapFromFunc

b19f71b

migrate MapFromFunc tests

57dacad

mjrodgers mentioned this pull request Jun 29, 2026

Migrate MapFromFunc from Hecke->AbstractAlgebra thofma/Hecke.jl#2290

Open

lgoettgens added the breaking label Jun 29, 2026

mjrodgers added 4 commits June 30, 2026 09:16

MapFromFunc tests

62cb56b

Don't need to scope MapHeader

b1eb68b

Fix tests, reorder arguments and return type for map_from_func

4c86299

use snake-case constructor for MapFromFunc

c7b8069

mjrodgers added 7 commits June 30, 2026 15:45

add `image_fn(f::HeckeMap)

090bf03

fix typo

6ab8315

fix doctest

3206924

fix doctests

642be69

fix typo

eb4e6a5

fix doctest error (lift)

3b52dfb

test with HeckeMap <: FunctionalMap

1825258

mjrodgers added 7 commits June 30, 2026 18:06

reorder abstract types

2936a13

remove image_fn test

bdf11d7

spacing in test was borked

2c80a32

MapFromFunc uses actual terse printing, fix in test

b95f533

fix julia-function -> Julia function in tests

c8fbf3e

avoid unnecessary use of image_fn/preimage_fn

ee82a0d

define preimage_fn for HeckeMap

485400c

mjrodgers added 11 commits July 3, 2026 12:37

doctest fix

d7ec044

doctest re-fix

ab07b98

doctest re-re-fix

48636a8

fix _is_attribute_storing_type

a3dfd59

MapFromFunc with inverse tests

aa877c2

use preimage_function instead of inverse_function for MapFromFunc

c7d00b7

AA uses inverse_fn for MapWithSection/`Retraction, so we match

c409596

test for last commit

ad0c3bc

typo in test

54d22df

test fix

27f389a

fix inverse_fn

c046414

mjrodgers force-pushed the MapFromFunc branch from 8039965 to c046414 Compare July 4, 2026 08:21

mjrodgers added 2 commits July 5, 2026 15:30

improve tests

b076fb1

test fix

afeeb2a

mjrodgers force-pushed the MapFromFunc branch from 5322225 to afeeb2a Compare July 5, 2026 14:10

mjrodgers added 3 commits July 5, 2026 16:27

revert change to Base.inv

edbd6e0

tests for MapHeader

0ee2ee5

test fix

c5bbbe1

mjrodgers mentioned this pull request Jul 6, 2026

Update map_from_func Nemocas/Nemo.jl#2342

Open

fingolfin reviewed Jul 8, 2026

View reviewed changes

	map_from_func(D, C, image_fn, inverse_fn) = MapFromFunc(D, C, image_fn, inverse_fn)
	@deprecated map_from_func(image_fn::Function, D, C) = MapFromFunc(D, C, image_fn)

-  image
-  preimage
+  image       # function (or other kind of object) used to actually compute images
+  preimage    # function (or other kind of object) used to actually compute preimages


		abstract type IdentityMap <: SetMap end

		abstract type HeckeMap <: FunctionalMap end

	abstract type HeckeMap <: FunctionalMap end
	abstract type HeckeMap <: SetMap end


		abstract type Map{D, C, S, T} <: SetElem end

		abstract type SetMap end

Uh oh!

Conversation

mjrodgers commented Jun 29, 2026 • edited by lgoettgens Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjrodgers commented Jun 29, 2026

Uh oh!

codecov Bot commented Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thofma commented Jun 29, 2026

Uh oh!

mjrodgers commented Jun 30, 2026

Uh oh!

thofma commented Jun 30, 2026

Uh oh!

mjrodgers commented Jun 30, 2026

Uh oh!

mjrodgers commented Jun 30, 2026

Uh oh!

mjrodgers commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fingolfin Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mjrodgers commented Jun 29, 2026 •

edited by lgoettgens

Loading

codecov Bot commented Jun 29, 2026 •

edited

Loading

mjrodgers commented Jun 30, 2026 •

edited

Loading

fingolfin Jul 8, 2026 •

edited

Loading