As @wdecker pointed out to me last week, this code is incorrect:

is_noetherian(M::Module) = is_noetherian(base_ring(M)) || throw(NotImplementedError(:is_noetherian, M))

What is true that a finitely generated module over a Noetherian ring is again Noetherian.

Some possible fixes:

Fix 1: check ngens

A crude fix might be to use this code:

is_noetherian(M::Module) = (is_noetherian(base_ring(M)) && is_finite(ngens(M))) || throw(NotImplementedError(:is_noetherian, M))

Of course in practice this won't make a difference for our current code; also, it is more likely that ngens will throw an error of its own than it returning inf.

Fix 2: introduce is_finitely_generated

A nicer fix might be to do introduce is_finitely_generated(::Module) and then write

is_noetherian(M::Module) = (is_noetherian(base_ring(M)) && is_finitely_generated(M)) || throw(NotImplementedError(:is_noetherian, M))

is_finitely_generated(M::Module) = throw(NotImplementedError(:is_finitely_generated, M))
is_finitely_generated(M::FPModule) = true

but once again in practice it won't be much different... But at least it is mathematically clean(er), maybe? And one could also have is_finitely_generated (and perhaps also is_finitely_presented?) methods for groups, rings and other objects (as long as it is clearly documented what kind of "generation" is meant in each case).

Fix 3: use method dispatch

Lastly, we could also do something like this:

is_noetherian(M::Module) = throw(NotImplementedError(:is_noetherian, M))
is_noetherian(M::FPModule) = is_noetherian(base_ring(M)) || throw(NotImplementedError(:is_noetherian, M))

Both fix 2 and fix 3 would probably break Oscar.jl, until its ModuleFP type gets the required methods, too. But it should be safe to first add the required methods in Oscar.jl, and only afterwards make this change, and declare it as "not a breaking change but rather a bug fix"... ah well...

We need to decide which way to go, then @emikelsons can implement the required changes :-).

CC @fieker @lgoettgens @thofma