Expand the common code for ideal implementations

[fingolfin]

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]

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]

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]

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.

[fingolfin]

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.

[thofma]

• 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]

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]

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.

[fingolfin]

I've revised this, and among other things dropped the DefaultIdealSet type.

In fact, I wonder: do we really want or need a parent for ideals? What's the usecase for that?

[thofma]

As domains or codomains of maps.

[fingolfin]

In which categories is that? Do we have any existing examples?

The ideal implementations I looked at in Oscar.jl all don't implement parent -- I guess your examples are in Hecke?

[thofma]

Here the example:

julia> using Oscar

julia> k, = quadratic_field(2);

julia> Ok = ring_of_integers(k);

julia> C, mC = class_group(Ok);

julia> domain(mC)
Z/1

julia> codomain(mC)
Set of ideals of maximal order of real quadratic field defined by x^2 - 2

julia> parent(1*Ok) == codomain(mC)
true

Probably implemented somewhere in Hecke.

Edit: I don't mind if this is dropped here, since we can just add it to the ideals we need it for.

[fingolfin]

OK, thanks for the example.

To be clear: no existing functionality is "dropped" here -- it's just that I wrote a generic DefaultIdealSet type which can be used as parent for ideals, and I believe this would in fact be sufficient for all ideal types (at least in the commutative ring setting). But that's simply yet another complication, so I decided to remove it from this PR. I could try to re-introduce it in a follow-up PR.

I also restricted a bunch of things in here to the commutative cases and essentially remove all references to the non-commutative setting -- with the exception of a few places that had NCRing or NCRingElem in them already. Because if I remove those, it might be breaking for something. (I am not opposed to removing it, I just don't want it to be done in this PR so that I hopefully can avoid adding the breaking label).

I have also put in the start for making the Generic.Ideal type opt-in (by providing the ideal and ideal_type methods for Integers, Generic.PolyRing and Generic.MPolyRing -- mainly to make the tests pass, but I am thinking of dropping this for MPolyRing again... that would include dropping a bunch of tests).

Oh and the new tests also discovered a bug: the new check_base_ring(I, J) method revealed a missing deepcopy_internal method for Generic.Ideal).

[thofma]

Sounds good

[codecov]

Codecov Report

❌ Patch coverage is 43.47826% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.02%. Comparing base (09520a8) to head (2ac4366).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
src/Ideal.jl	43.24%	21 Missing ⚠️
src/generic/Ideal.jl	33.33%	2 Missing ⚠️
src/Poly.jl	50.00%	1 Missing ⚠️
src/generic/MPoly.jl	50.00%	1 Missing ⚠️
src/julia/Integer.jl	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2108      +/-   ##
==========================================
- Coverage   88.08%   88.02%   -0.06%     
==========================================
  Files         127      127              
  Lines       31782    31800      +18     
==========================================
- Hits        27994    27991       -3     
- Misses       3788     3809      +21     

☔ View full report in Codecov by Sentry.
📢 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.

[fingolfin]

Unfortunately Oscar also implements such a hash method for ideals, and a bunch of others, making this breaking. I'll make a matching PR.

And I guess it means this PR is breaking after all sigh.