AbstractAlgebra.jl icon indicating copy to clipboard operation
AbstractAlgebra.jl copied to clipboard

Define is_homogeneous for MPolyRingElem

Open fingolfin opened this issue 1 year ago • 9 comments

fingolfin avatar Apr 03 '24 22:04 fingolfin

Tests not passing

thofma avatar Apr 04 '24 05:04 thofma

Did you discuss this with the Geometry crowd? There might be a reason this function did not exist ... (other than oversight)

fieker avatar Apr 04 '24 06:04 fieker

It didn't occur to me, given that it is already part of the AA documentation which promises it as being available for all MPolyRingElem subtypes. To quote docs/src/mpolynomial.md:

### Homogeneous polynomials

It is possible to test whether a polynomial is homogeneous with respect to the standard grading using the function

```@docs
is_homogeneous(x::MPolyRingElem{T}) where T <: RingElement
```

But sure, we can discuss it -- CC @ederc @wdecker @jankoboehm @simonbrandhorst @afkafkafk13 (I probably forgot someone, sorry)

fingolfin avatar Apr 04 '24 06:04 fingolfin

To be clear, I think the question here is whether we should keep exporting AbstractAlgebra.Generic.is_homogeneous as AbstractAlgebra.is_homogeneous (which therefore also exists in Oscar).

I note that in Oscar, there is also a function _is_homogeneous which does more or less what the changes in this PR do for is_homogeneous.

fingolfin avatar Apr 04 '24 07:04 fingolfin

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 86.75%. Comparing base (bae82ed) to head (496bc96). Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1649   +/-   ##
=======================================
  Coverage   86.75%   86.75%           
=======================================
  Files         114      114           
  Lines       29660    29659    -1     
=======================================
  Hits        25731    25731           
+ Misses       3929     3928    -1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Apr 04 '24 07:04 codecov[bot]

I of course forgot to tag @hechtiderlachs sorry!

fingolfin avatar Apr 05 '24 09:04 fingolfin

We discussed this a bit and would suggest that we have (to avoid confusion with the OSCAR function)

is_homogeneous with two required arguments, polynomial and weight vector

and a convenience function

is_standard_homogeneous for a polynomial to cover the typical use case.

That would also capture the setting we can realize here. Would that be fine?

jankoboehm avatar Apr 05 '24 12:04 jankoboehm

Just a random comment: Maybe is_homogenous_standard? Grammar is worse, but easier to discover with tabcompletion.

Also, we could make is_homogeneous(f) for an element f of a non-graded polynomial ring spill out a useful error message.

thofma avatar Apr 05 '24 12:04 thofma

Just a random comment: Maybe is_homogenous_standard? Grammar is worse, but easier to discover with tabcompletion.

This does not make it more understandable. You still need to know what the 'standard' refers to, if you want to make sense of suggested completion.

Also, we could make is_homogeneous(f) for an element f of a non-graded polynomial ring spill out a useful error message.

I am very much in favour of this suggestion. The error message should state that the user could either specify a grading or use 'is_standard_homogeneous' (or whatever name it bears at that moment).

afkafkafk13 avatar Apr 05 '24 13:04 afkafkafk13