Is_unit and is_nilpotent (with tests) #1933

JohnAAbbott · 2024-12-17T16:12:52Z

Still a draft

src/LaurentMPoly.jl

src/MPoly.jl

src/Poly.jl

src/Rings.jl

thofma · 2024-12-18T07:18:59Z

I don't think we can rely on any order when iterating over the coefficients. The only guarantee that we have is that coefficients and exponent[_vectors] have the same order. E.g., this is the implementation of constant_coefficient:

function constant_coefficient(p::MPolyRingElem{T}) where T <: RingElement
   if !iszero(p)
      for (c, v) in zip(coefficients(p), exponent_vectors(p))
         if iszero(v)
            return c
         end
      end
   end
   return zero(base_ring(p))
end

…ilpotent

JohnAAbbott · 2024-12-19T12:16:22Z

I notice that there are tests of is_unit for Laurent series: for instance

./generic/LaurentSeries-test.jl:156:   @test is_unit(-1 + x + 2x^2)

Should I disable these tests? I have not yet checked which code would be called...
Perhaps it is simpler to leave things be?

…/generic/LaurentMPoly

codecov · 2024-12-19T13:38:46Z

Codecov Report

Attention: Patch coverage is 95.52239% with 3 lines in your changes missing coverage. Please review.

Project coverage is 87.89%. Comparing base (747df7d) to head (c10f880).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/NCRings.jl	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1933      +/-   ##
==========================================
- Coverage   88.21%   87.89%   -0.33%     
==========================================
  Files         119      119              
  Lines       30384    30397      +13     
==========================================
- Hits        26803    26717      -86     
- Misses       3581     3680      +99

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

fingolfin · 2024-12-19T15:05:21Z

I notice that there are tests of is_unit for Laurent series: for instance
./generic/LaurentSeries-test.jl:156:   @test is_unit(-1 + x + 2x^2)
Should I disable these tests? I have not yet checked which code would be called... Perhaps it is simpler to leave things be?

I don't understand. Why would you disable the tests?

Anyway, checking what code is called shouldn't be hard with @edit and @less, right?

fingolfin · 2024-12-19T15:07:48Z

src/MPoly.jl

+  is_constant(f) && return is_unit(constant_coefficient(f))
+  # Here deg(f) > 0; over an integral domain, non-constant polynomials are never units:
+  is_domain_type(elem_type(coefficient_ring(f))) && return false
+  for (c, expv) in zip(coefficients(f), exponent_vectors(f))


Can we add a comment explaining the math? E.g.

Suggested change

for (c, expv) in zip(coefficients(f), exponent_vectors(f))

# A polynomial over a commutative ring is a unit if and only if its

# constant term is a unit and all other coefficients are nilpotent.

# See URL/BOOKREF/SOMETHING for a proof.

for (c, expv) in zip(coefficients(f), exponent_vectors(f))

Obviously with a suitable source inserted.

src/MPoly.jl

JohnAAbbott · 2024-12-19T15:08:55Z

I don't understand. Why would you disable the tests?

In general, we cannot always decide if a *-series is a unit: e.g. if no terms of the series are known, or if the only known terms all have nilpotent coefficients...

fingolfin · 2024-12-19T15:10:57Z

src/MPoly.jl

+function is_nilpotent(f::T) where {T<:MPolyRingElem}
+  is_domain_type(elem_type(coefficient_ring(f))) && return is_zero(f)


Suggested change

function is_nilpotent(f::T) where {T<:MPolyRingElem}

is_domain_type(elem_type(coefficient_ring(f))) && return is_zero(f)

function is_nilpotent(f::MPolyRingElem{T}) where {T}

is_domain_type(T) && return is_zero(f)

would be IMHO more idiomatic (and simply much shorter to read).

The same could of course be used in is_unit above and elsewhere.

OK, I'll try. I have found is_domain_type to be quite obstreperous, which is why I have used work-arounds. Maybe I should update...

Could you clarify what you found problematic about is_domain_type? What are you "working around"?

src/MPoly.jl

src/Rings.jl

fingolfin · 2024-12-19T15:19:40Z

src/NCRings.jl

+function is_nilpotent(a::T) where { T <: NCRingElem }
+  is_domain_type(typeof(a)) && return is_zero(a)


Suggested change

function is_nilpotent(a::T) where { T <: NCRingElem }

is_domain_type(typeof(a)) && return is_zero(a)

function is_nilpotent(a::T) where {T <: NCRingElem}

is_domain_type(T) && return is_zero(a)

Julia convention is to write {T <: NCRingElem} not { T <: NCRingElem }

I can do what you prefer, but there are lots of other functions which use different spacing in the where clause e.g. no curly brackets, no spaces around <:,...

Sure. But I am not asking you to reformat the code, just to not introduce yet more variation. I just grepped the code base for AA and I found exactly one hit for where { (i.e., with a space after the opening curly brace), and that was in code that was (a) copied from another project and (b) trying to highlight a very specific pattern (i.e. it makes sense, locally, to format it that way).

Let's not introduce this new variation without need in new code, yes?

fingolfin · 2024-12-19T16:38:23Z

As I expressed on several occasions, it'd be perfectly fine to ignore series completely for now, focus on getting the rest right, then after that is ready and merged, think more about dealing with series. So that we get the "easier" (not easy) bits done and usable, instead of getting bogged down in the more complicated (and less useful for us, currently) bits.

But since you are asking: we do implement is_zero and is_one for these rings, no? Then my view point would be that there is a clear definition for is_nilpotent and is_unit, too, no? If there is a positive integer k such that is_zero(x^k) holds, then x is nilpotent. And likewise, if there is a y such that is_one(x*y) then x is a unit.

Now whether this is useful is another question -- which is part of why I'd be OK with us raising an error if one of these functions is called on a series element.

fingolfin · 2024-12-19T16:38:43Z

BTW, note that doctests in docs/src/polynomial.md are failing

fingolfin · 2024-12-19T16:28:17Z

src/julia/Rational.jl

@@ -40,6 +40,8 @@ one(::Rationals{T}) where T <: Integer = Rational{T}(1)

 is_unit(a::Rational) = a != 0

+is_nilpotent(a::Rational) = (a == 0)


fingolfin · 2024-12-19T16:28:21Z

src/julia/Integer.jl

@@ -42,6 +42,8 @@ one(::Integers{T}) where T <: Integer = T(1)

 is_unit(a::Integer) = a == 1 || a == -1

+is_nilpotent(a::Integer) = is_zero(a)


fingolfin · 2024-12-19T16:30:37Z

test/generic/MPoly-test.jl

+  @test is_nilpotent(-x)
+  @test is_nilpotent(x+y)
+  @test is_nilpotent(x-y)
+  @test is_nilpotent(x*y)


Instead of adding these tests (which are very similar each time) into multiple places, it would be better to test them to the general ring interface test suite in test/Rings-conformance-tests.jl so that the tests get run for all our ring implementations (well, at least all that use the conformance tests).

Of course this may then reveal further issues, but so be it.

fingolfin · 2024-12-19T16:39:48Z

I'd like to add that despite my many minor nitpicks, overall this PR seems to be shaping up and headed in the right direction. Thank you.

src/MPoly.jl

JohnAAbbott added 5 commits December 16, 2024 16:54

Added generic/default defn of is_nilpotent

27ad97f

Merge branch 'master' of github.com:JohnAAbbott/AbstractAlgebra.jl

37ca35c

Corrected several typos (all on one line)

bceb3ad

New impls of is_unit & is_nilpotent

a859a05

Tests for new is_unit & is_nilpotent

6dcb4a3

fingolfin reviewed Dec 17, 2024

View reviewed changes

src/LaurentMPoly.jl Outdated Show resolved Hide resolved

fingolfin reviewed Dec 17, 2024

View reviewed changes

src/MPoly.jl Outdated Show resolved Hide resolved

fingolfin reviewed Dec 17, 2024

View reviewed changes

src/MPoly.jl Outdated Show resolved Hide resolved

src/MPoly.jl Outdated Show resolved Hide resolved

src/MPoly.jl Outdated Show resolved Hide resolved

src/Poly.jl Outdated Show resolved Hide resolved

src/Poly.jl Show resolved Hide resolved

src/Rings.jl Outdated Show resolved Hide resolved

JohnAAbbott and others added 14 commits December 18, 2024 12:25

Add impls of is_unit & is_nilpotent

bcdb3c3

Copied defns of is_unit & is_nilpotent from Nemo

c9e711b

Merge branch 'Nemocas:master' into master

68afc5b

Added is_nilpotent

d4343d1

Merge branch 'master' of github.com:JohnAAbbott/AbstractAlgebra.jl

e8c8c3e

Commented out old broken impl of is_unit

3ecd278

is_unit for PolyRingElem & for NCPolyRing elem merged; ditto for is_n…

81ec784

…ilpotent

Added is_nilpotent(::Integer)

1a0cd07

Moved is_unit & is_nilpotent to NCPoly.jl; deleted cruft

2ad7109

Moved is_unit & is_nilpotent here from Poly.jl; improvements

8875511

Moved default is_nilpotent from Rings.jl to NCRings.jl

af88767

Improved is_unit & is_nilpotent (from feedback)

ff9a4d9

Now using neater test for zero ring

3cafa67

Inserted missing test is_unit(constant_coeff...)

5def6e7

JohnAAbbott added 6 commits December 19, 2024 13:59

Added is_nilpotent for Rational

20b2fac

Removed old is_unit test; fixed typo

19ac0ea

Fixed typo

ff41535

Removed outdated is_unit test

fd49a42

Removed outdated test for is_unit

d2978c2

Moved new defns of is_unit, is_nilpotent from src/LaurentMPoly to src…

76bbaa6

…/generic/LaurentMPoly

JohnAAbbott added 2 commits December 19, 2024 14:14

Removed old, buggy impl of is_unit

23ad37d

Removed old, buggy impl of is_unit

3228e1d

fingolfin reviewed Dec 19, 2024

View reviewed changes

src/MPoly.jl Outdated Show resolved Hide resolved

fingolfin reviewed Dec 19, 2024

View reviewed changes

src/MPoly.jl Show resolved Hide resolved

fingolfin reviewed Dec 19, 2024

View reviewed changes

src/Rings.jl Outdated Show resolved Hide resolved

fingolfin reviewed Dec 19, 2024

View reviewed changes

JohnAAbbott added 6 commits December 19, 2024 18:00

Better layout in a comment

ed26e8c

Minor improvements/tidying

e664624

Neater calls to is_domain_type

4707518

Neater calls to is_domain_type

f135f15

Deleted blank line

2cfde89

Moved is_nilpotent up to src/Residue.jl

c10f880

fingolfin reviewed Dec 19, 2024

View reviewed changes

src/MPoly.jl Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Is_unit and is_nilpotent (with tests) #1933

Is_unit and is_nilpotent (with tests) #1933

JohnAAbbott commented Dec 17, 2024

thofma commented Dec 18, 2024

JohnAAbbott commented Dec 19, 2024

codecov bot commented Dec 19, 2024 •

edited

Loading

fingolfin commented Dec 19, 2024

fingolfin Dec 19, 2024

JohnAAbbott commented Dec 19, 2024

fingolfin Dec 19, 2024

JohnAAbbott Dec 19, 2024

fingolfin Dec 19, 2024

fingolfin Dec 19, 2024

JohnAAbbott Dec 19, 2024

fingolfin Dec 19, 2024

fingolfin commented Dec 19, 2024

fingolfin commented Dec 19, 2024

fingolfin Dec 19, 2024

fingolfin Dec 19, 2024

fingolfin Dec 19, 2024

fingolfin commented Dec 19, 2024

-  for (c, expv) in zip(coefficients(f), exponent_vectors(f))
+  # A polynomial over a commutative ring is a unit if and only if its
+  # constant term is a unit and all other coefficients are nilpotent.
+  # See URL/BOOKREF/SOMETHING for a proof.
+  for (c, expv) in zip(coefficients(f), exponent_vectors(f))

		function is_nilpotent(f::T) where {T<:MPolyRingElem}
		is_domain_type(elem_type(coefficient_ring(f))) && return is_zero(f)

		function is_nilpotent(a::T) where { T <: NCRingElem }
		is_domain_type(typeof(a)) && return is_zero(a)

		@@ -40,6 +40,8 @@ one(::Rationals{T}) where T <: Integer = Rational{T}(1)

		is_unit(a::Rational) = a != 0

		is_nilpotent(a::Rational) = (a == 0)

		@@ -42,6 +42,8 @@ one(::Integers{T}) where T <: Integer = T(1)

		is_unit(a::Integer) = a == 1 \|\| a == -1

		is_nilpotent(a::Integer) = is_zero(a)

Is_unit and is_nilpotent (with tests) #1933

Are you sure you want to change the base?

Is_unit and is_nilpotent (with tests) #1933

Conversation

JohnAAbbott commented Dec 17, 2024

thofma commented Dec 18, 2024

JohnAAbbott commented Dec 19, 2024

codecov bot commented Dec 19, 2024 • edited Loading

Codecov Report

fingolfin commented Dec 19, 2024

Choose a reason for hiding this comment

JohnAAbbott commented Dec 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin commented Dec 19, 2024

fingolfin commented Dec 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin commented Dec 19, 2024

codecov bot commented Dec 19, 2024 •

edited

Loading