Skip to content

Make univariate Laurent polynomials over a field pass the TestSuite#37778

Merged
vbraun merged 2 commits intosagemath:developfrom
tscrim:rings/laurent_gcd_match_xgcd
Apr 27, 2024
Merged

Make univariate Laurent polynomials over a field pass the TestSuite#37778
vbraun merged 2 commits intosagemath:developfrom
tscrim:rings/laurent_gcd_match_xgcd

Conversation

@tscrim
Copy link
Copy Markdown
Collaborator

@tscrim tscrim commented Apr 10, 2024

The output of xgcd does not match the gcd output, but this is required by #17671. We change the computation to make these match. We also fix another bug as it should also take non-Laurent polynomial inputs to xgcd.

We also implement the euclidean_domain() method.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@enriqueartal
Copy link
Copy Markdown
Contributor

Before looking at how is coded, it is right that p.gcd(q) should be equal to p.xgcd(q)[0]. What is less clear to me is the choice of the gcd up to unit. Whenever there is a canonical choice this should be done (for field coefficients monic with non-zero independent term). For example, now (-1).gcd(-1) gives 1. Should x.gcd(x) be 1 for Laurent polynomials?

@tscrim
Copy link
Copy Markdown
Collaborator Author

tscrim commented Apr 10, 2024

The Laurent polynomial gcd was done so that for g = gcd(a, b) the degrees of a / g and b / g are minimized (in particular, at least one of them has valuation 0). Your proposal would be to make the gcd have valuation 0, correct? Both choices are canonical to me, but I slightly like more the current gcd choice.

I don't see any mathematical difference with your proposal outright. Can you explain more? Is it more computational?

@enriqueartal
Copy link
Copy Markdown
Contributor

I am not sure if my proposal is better. In sage for the integers, the gcd of a negative number and itself is positive, and I guess it was done since there is a canonical choice of an integer up to a unit. As you notice, for a non-zero Laurent polynomial (with coefficients in a field or in general in a ring where the canonical choice can be done), there is a canonical choice of valuation 0. More than computational issues, I think of that the canonical choice is better and it has been done in other case.

@tscrim
Copy link
Copy Markdown
Collaborator Author

tscrim commented Apr 10, 2024

It's basically just you like it better than anything mathematical (say, something algebro-geometric related). That's not unreasonable.

However, I think we should keep it the way it is because it will match on polynomials. This is parallel to how the gcd for QQ is done to match ZZ (and other fraction fields). I would say it is less surprising for the more casual user.

I am not sure which would be faster or less code. I imagine both versions would be basically the same...

@enriqueartal
Copy link
Copy Markdown
Contributor

I basically agree with you. Maybe not right now, but I wonder that if we may have both with a keyword as normalize on both gcd and xgcd for Laurent polynomials.

@github-actions
Copy link
Copy Markdown

Documentation preview for this PR (built with commit f87b6fd; changes) is ready! 🎉

@tscrim
Copy link
Copy Markdown
Collaborator Author

tscrim commented Apr 11, 2024

Thank you.

vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 20, 2024
sagemathgh-37778: Make univariate Laurent polynomials over a field pass the TestSuite
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

The output of `xgcd` does not match the `gcd` output, but this is
required by sagemath#17671. We change the computation to make these match. We
also fix another bug as it should also take non-Laurent polynomial
inputs to `xgcd`.

We also implement the `euclidean_domain()` method.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37778
Reported by: Travis Scrimshaw
Reviewer(s): Enrique Manuel Artal Bartolo, Travis Scrimshaw
@vbraun vbraun merged commit 2274139 into sagemath:develop Apr 27, 2024
@tscrim tscrim deleted the rings/laurent_gcd_match_xgcd branch April 28, 2024 08:33
vbraun pushed a commit to vbraun/sage that referenced this pull request May 11, 2024
sagemathgh-37719: Refactor ring categories
    
A somewhat large refactoring of some of the auld classes for rings, and
related categories

- introducing a new category of Noetherian rings
- moving some methods in appropriate categories
- fixing necessary details

### 📝 Checklist

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

Depends on sagemath#37778
    
URL: sagemath#37719
Reported by: Frédéric Chapoton
Reviewer(s): Matthias Köppe, Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this pull request May 12, 2024
sagemathgh-37719: Refactor ring categories
    
A somewhat large refactoring of some of the auld classes for rings, and
related categories

- introducing a new category of Noetherian rings
- moving some methods in appropriate categories
- fixing necessary details

### 📝 Checklist

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

Depends on sagemath#37778
    
URL: sagemath#37719
Reported by: Frédéric Chapoton
Reviewer(s): Matthias Köppe, Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this pull request May 12, 2024
sagemathgh-37719: Refactor ring categories
    
A somewhat large refactoring of some of the auld classes for rings, and
related categories

- introducing a new category of Noetherian rings
- moving some methods in appropriate categories
- fixing necessary details

### 📝 Checklist

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

Depends on sagemath#37778
    
URL: sagemath#37719
Reported by: Frédéric Chapoton
Reviewer(s): Matthias Köppe, Travis Scrimshaw
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants