Skip to content

Expand the common code for ideal implementations #2108

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fingolfin
Copy link
Member

This includes the addition of a DefaultIdealSet struct type which can be used with zero overhead, and hopefully can simply be used as parent type for all ideal implementations (in Hecke, Oscar or wherever) down the road.

Also semi-documented the interfaces to be implemented (the list is surely incomplete but better than nothing).

This includes the addition of a `DefaultIdealSet` struct type
which can be used with zero overhead, and hopefully can simply
be used as parent type for all ideal implementations (in Hecke,
Oscar or wherever) down the road.

Also semi-documented the interfaces to be implemented (the list
is surely incomplete but better than nothing).
@thofma
Copy link
Member

thofma commented Jun 17, 2025

I have mainly questions about similar (which is becoming part of the interface, since similar is exported by default?):

  • Will this be the official way to create new ideals? I'd personally rather see people write ideal(base_ring(I), new_elements) than similar(I, new_elements).
  • Is this only for internal use to account for multiple ideal implementations with the same base ring?
  • If left/right/two-sided ideals for a specific ring share the exact type, how well does this work with the DefaultIdealSet?

@fingolfin
Copy link
Member Author

All good questions, I've been pondering them as well, and I thought I should make this PR precisely so that we have a somewhat better base for discussing them.

Let's start with the non-commutative situation, I'll reply regarding similar in a separate post:

Right now as far as I can tell this is completely open and unspecified, and we should at least have a rough plan before proceeding. Do we have any serious non-commutative rings with ideals in any of our packages?

I realize there is some GB code for free associative algebras both here in AA (due to @julien-schanz @Sequenzer ) and in Oscar (based on Singular code). But the code in AA does not seem to even try to define an ideal type (and worse, it does not even seem to specify what kind of GB it computes: two-sided or one-sided...).

The code in Oscar defines a type FreeAssociativeAlgebraIdeal which explicitly is for two-sided ideals, and is a subtype of AbstractAlgebra.Ideal.

Do we have any other existing code for non-commutative ideals?

Anyway, we now have some options how to proceed;

  1. We could say that ideal_type, Ideal, DefaultIdealSet etc. are only for use with two-sided ideals
    • this is kinda what the current code assumes, as e.g. both x*R and R*x are mapped to ideal(R, [x])
    • it fits with Oscar's FreeAssociativeAlgebraIdeal
  2. or we assume they are only for one-sided ideals
  3. or we try to somehow support both kinds of ideals (maybe in fact three types: two-sided, left, and right ideals)

Regardless of which option we go, we need to agree on APIs if we want to support all kinds of ideals. Probably a good idea to also look at e.g. Sage and Magma for inspiration, which I have not yet done; just some general thoughts

  • how to determine whether an ideal is two or one sided, resp. right or left sided
    • via the type? probably too limited?
    • via type traits? is_left_ideal, is_right_ideal, is_twosided_ideal / is_bi_ideal (maybe also is_ideal ?)
      • (mathematically, a two-sided ideal is also a left-sided ideal; what about our traits? I guess we could have is_onesided / is_twosided and separately is_left / is_right
  • how would one go about constructing a left/right/twosided ideal? For the latter we have ideal(R,xs); maybe left_ideal(R,xs) and right_ideal(R,xs) ?

I think to answer this well, we need actual applications involving one-sided ideals...

Personally I think that option 1. is the way to go:

  • by far the easiest to get right (as in: "correct")
  • requires the least amount of code changes
  • minimizes risk of breakage of existing code
  • fits with the philosophy that it's best to design something if there is an actual use case.

BTW, do we know someone who currently has an actual usecase for one-sided ideals? Who would be interested in discussing an implementing a design for them together with us? That'd be great!

@fingolfin
Copy link
Member Author

Regarding similar: this is a late addition as I keep fluctuating between two (well, three?) different approaches to integrating Generic.Ideal with all of this. Specifically, in those functions which take an ideal and construct a new one from it (e.g. sum of two ideals), how do we do this:

  1. via ideal(R, xs) (that's how I is without the second commit in this PR)
    • but then we kinda should define such methods for Generic.Ideal, but for which base rings? All? Just a fixed list of rings defined in AA (which?)
    • and of course this then also needs corresponding ideal_type methods which return Generic.Ideal
  2. or we use similar as done with the second commit
  3. or we kinda punt on it and say "tough luck Generic.Ideal", we don't like you anyway and won't make any of this work for you.

While I understand that there is some loathing for Generic.Ideal, maybe it's actually not that bad? The main problem I see with it is that instead of defining methods only for cases that are known to work, it just runs code blindly, in the hope that it maybe works -- apparently the idea is to let you use it "if you know what you are doing". Which... I see where it comes from, but it is problematic because it is so easy to misuse.

But it seems OK for "toy" ideals over Z, fields, polynomial rings over fields? Or am I missing something?

BTW if we go with a version of option 1, then I can also replace Generic.IdealSet by DefaultIdealSet. In fact that's what I did first, but then I started to second guess myself... Hrm.

return ideal(parent(x), x; kw...)
function *(I::Ideal{T}, p::T) where T <: RingElement
iszero(p) && return similar(I, T[])
return similar(I, [v*p for v in gens(I)])
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so here, if we assume Ideal means "two sided", and also "there is one (two-sided) ideal type per ring", and also ideal(R, xs) produces that ideal type, then we can write here

Suggested change
return similar(I, [v*p for v in gens(I)])
return ideal(base_ring(I), [v*p for v in gens(I)])

and indeed that's what I did before the second commit in this PR, and I'd be happy to back to it if we can agree on this scheme.

@fingolfin
Copy link
Member Author

BTW a primary motivation is that I looked at ideal implementation in Oscar.jl and found

  • a lot of duplicate code
  • type piracy
  • incorrect code
  • needlessly inefficient code when somewhere else someone already wrote a more efficient version
  • uneven APIs (some ideal types support feature A but not B, others support B but not A, some don't support either, some both)

So I actually started with a partial PR for Oscar, then decided to shelf that for now, and instead work on the fundamental in AA. Once we have agreed on something here, I plan to proceed to make PRs for Hecke and Oscar to adjust things to it.


function Base.:*(I::T, J::T) where {T <: Ideal}
check_base_ring(I, J)
return similar(I, [x*y for x in gens(I) for y in gens(J)])
Copy link
Member

@thofma thofma Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong for two-sided ideals of noncommutative rings.

@thofma
Copy link
Member

thofma commented Jun 17, 2025

  • Taking FreeAssociativeAlgebraIdeal as the standard for ideals for non-commutative rings seems rather limiting. We have existing code for non-commutative ideals in Hecke (https://github.com/thofma/Hecke.jl/blob/master/src/AlgAss/Ideal.jl, https://github.com/thofma/Hecke.jl/blob/master/src/AlgAssAbsOrd/Ideal.jl) and Oscar (https://github.com/oscar-system/Oscar.jl/blob/master/src/Rings/PBWAlgebra.jl). The Hecke code has been in use for years. There are plenty of applications of one-sided ideals. I also have more ideal stuff in my finite ring code, but this is still private.
  • They are created using left_ideal, right_ideal and twosided_ideal. In Hecke we also have ideal(R, ...; side = :left) for legacy reasons. Similar to left- and right-modules, I would argue that there should not be an unqualified ideal(R::NCRing, xs). It was an error to add this in the first place.
  • The Hecke ideals have the "sidedness" as a runtime flag. It is stored per side with true/false/unkown and upon querying it, it will be determined. There is only one type for everything.
  • For reasons I don't understand, in Oscar a type parameter (0, 1, 2) is used to distinguish the "sidedness". (I doubt it is for performance reasons. It seems to also not be used for dispatch.)

I personally would remove anything related to non-commutative things from the "generic"/"fallback" stuff. I think the FreeAssociativeAlgebraIdeal is not a good model for what an interface in the non-commutative case should like in general.

@thofma
Copy link
Member

thofma commented Jun 17, 2025

I think we can remove Generic.Ideal. If not, we should keep it being not available via ideal. We did this on purpose and the reasons for doing it are all still valid: We have better things over Z and PIDs in general. For multivariate polynomial rings, it

  • is probably not better than Singular,
  • does not play nice with the groebner_basis infrastructure, so it is useless for any other construction that the commutative algebra people like to do
  • is an undocumented maintenance burden

@fingolfin
Copy link
Member Author

I'd be fine with restricting all the work in here to the commutative setting, no problem.

I wasn't really aware of the non-commutative ideals in Hecke, cool, I'll have a look eventually. But for now I am mainly concerned about having the commutative ideals suck less in terms of consistent user experience.

I can also look into removing Generic.Ideal, my heart is not tied to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants