Skip to content
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

Define is_homogeneous for MPolyRingElem #1649

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fingolfin
Copy link
Member

No description provided.

@thofma
Copy link
Member

thofma commented Apr 4, 2024

Tests not passing

@fieker
Copy link
Contributor

fieker commented Apr 4, 2024

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

@fingolfin
Copy link
Member Author

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
Copy link
Member Author

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.

Copy link

codecov bot commented Apr 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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     

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

@fingolfin
Copy link
Member Author

I of course forgot to tag @HechtiDerLachs sorry!

@jankoboehm
Copy link

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?

@thofma
Copy link
Member

thofma commented Apr 5, 2024

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.

@afkafkafk13
Copy link

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).

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.

None yet

5 participants