`sub` and `quo` for `ModuleFP` are not type stable

[lgoettgens]

Due to the parameter task::Symbol that specifies the additional data to be returned, type inference gives up when encountering sub or quo.

Possible solutions:

1. different function names for different task values
2. changing the type of task from Symbol to Val{...}. All library code should call this version. For interactive use, we can still provide the variant with Symbol that then calls the other one.

[fieker]

On Mon, Dec 18, 2023 at 08:02:08AM -0800, Lars Göttgens wrote:

Due to the parameter task::Symbol that specifies the additional data to be returned, type inference gives up when encountering sub or quo.

Possible solutions:

1. different function names for different task values
2. changing the type of task from Symbol to Val{...}. All library code should call this version. For interactive use, we can still provide the variant with Symbol that then calls the other one.

I was assuming that sub and quo always return 2 things: object and map. In the rare cases that I really only want one, I throw the other away. So far I have not really encountered a situation (which might exist) where the creation of the map say is expensive.

The "other" question: what is the downside of this type instability? (other than by principle)? Nothing you can do to this modules is faster than the generic dispatch. The Val{} approach is theortically fine, but extremely hostile to users, as in almost no-one will be able to read or use this - we've used this in parts of Hecke. In library code we could also use type asserts after the fact to get julia back on track

-- Reply to this email directly or view it on GitHub: https://github.com/oscar-system/Oscar.jl/issues/3118 You are receiving this because you are subscribed to this thread.

Message ID: @.***>

[lgoettgens]

I was assuming that sub and quo always return 2 things: object and map. In the rare cases that I really only want one, I throw the other away. So far I have not really encountered a situation (which might exist) where the creation of the map say is expensive.

If that is the case, we can remove the task variants that only return one of them, right?

The "other" question: what is the downside of this type instability? (other than by principle)? Nothing you can do to this modules is faster than the generic dispatch. The Val{} approach is theortically fine, but extremely hostile to users, as in almost no-one will be able to read or use this - we've used this in parts of Hecke. In library code we could also use type asserts after the fact to get julia back on track

The big downside of this for me right now is that it makes it incredibly hard to find the "real" type instability in code. Type asserts would be fine as well for me, basically anything that makes type inference on code (that might create a subquo) succeed.

[fingolfin]

We just discussed this here and would suggest to remove the optional :task argument for sub and quo.

If there is need for some applications of modules to really only get one or the other, this can be done via an internal helper...

@HechtiDerLachs also pointed out that there is an open issue #2999 because in some cases creating the morphism for sub/quo takes 90% of the time -- though it shouldn't, i.e. it seems this is more a bug and not something justifying an API change.

[fingolfin]

For starters having a list of methods that need to be updated might be helpful?

For the modules, @HechtiDerLachs will have a look this afternoon.

[jankoboehm]

I agree with @fieker 's comment above that the type instabiliy induced by the task keyword is a not really an issue (and this approach was also suggested to us to follow for a long time). Nevertheless it is decided now to change it, also in other contexts, to get rid of it and it is probably a cleaner thing.

But, if we do that then we should please be consistent with what is done with other constructions creating an object and a corresponding map. I am referring in particular to the discussion on sums and products #3059.

I agree with @wdecker that for algebraic geometry, there are use cases where only the object is needed (for example if you want only numerical information of that like that rank), and that there are situations and examples where constructing the map (however efficient it might be) will be expensive. So we should always have a function constructing the object, as well as a function returning the object and the map.

I am not totally sure what is the outcome of #3059, but I dont like submodule and sub, especially since type information which is obvious from the input should better not be used in a function name. The clean thing would be probably: sub, sub_with_injection, quo, quo_with_projection, or if we want sub and quo to also do the maps, then sub_object and sub, as well as quo_object and quo.

[fieker]

Proposal (decided on!!!!) sub, quo return the map, always sub_object, quo_object are map free

[fieker]

DONE!!!!