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.

Sign up for GitHub

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

Jump to bottom

Allow custom degree in random_element of polynomial quotient ring #37268

Merged
merged 2 commits into from
Feb 25, 2024
Merged

Allow custom degree in random_element of polynomial quotient ring #37268

merged 2 commits into from
Feb 25, 2024

Conversation

aimileus
Copy link
Contributor

@aimileus aimileus commented Feb 9, 2024 •
edited

Currently, the random_element function of the polynomial quotient ring passes all arguments through to the base ring. However, the random_element function itself passes a default degree to the base ring, based on the degree of the quotient ring. So, overwriting the degree in kwargs gives a TypeError. Therefore, detect the degree argument manually, such that the user can overrride the default degree.

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • 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

S17A05
S17A05 reviewed Feb 11, 2024
View reviewed changes
Copy link
Contributor

@S17A05 S17A05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two small things, otherwise LGTM.

src/sage/rings/polynomial/polynomial_quotient_ring.py Outdated Show resolved Hide resolved
src/sage/rings/polynomial/polynomial_quotient_ring.py Outdated Show resolved Hide resolved
@aimileus aimileus force-pushed the pol-quotient-deg branch 3 times, most recently from d3dac7d to f803b69 Compare February 12, 2024 11:27
S17A05
S17A05 reviewed Feb 13, 2024
View reviewed changes
Copy link
Contributor

@S17A05 S17A05 left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the future, please don't force push big changes and instead put them into a new commit, otherwise it makes the review process harder because everything has to be re-checked again.

@aimileus
Allow custom degree in random_element of polynomial quotient ring
3ff46e3 
Currently, the `random_element` function of the polynomial quotient ring passes
all arguments through to the base ring. However, the random_element function
itself passes a default degree to the base ring, based on the degree of the
quotient ring. So, overwriting the degree in kwargs gives a TypeError.
Therefore, detect the `degree` argument manually, such that the user can
overrride the default degree.
@aimileus
Fix typo
0e24c1f
@aimileus
Copy link
Contributor Author

I fixed the typo, and rebased on develop.

@github-actions GitHub Actions
Copy link

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

S17A05
S17A05 approved these changes Feb 16, 2024
View reviewed changes
Copy link
Contributor

@S17A05 S17A05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM.

vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 18, 2024
sagemathgh-37268: Allow custom degree in random_element of polynomi…
259c97c 
…al quotient ring

    
Currently, the `random_element` function of the polynomial quotient ring
passes all arguments through to the base ring. However, the
random_element function itself passes a default degree to the base ring,
based on the degree of the quotient ring. So, overwriting the degree in
kwargs gives a TypeError. Therefore, detect the `degree` argument
manually, such that the user can overrride the default degree.

<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [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.
- [x] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#37268
Reported by: Emiel Wiedijk
Reviewer(s): Emiel Wiedijk, Sebastian A. Spindler
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 19, 2024
sagemathgh-37268: Allow custom degree in random_element of polynomi…
73907de 
…al quotient ring

    
Currently, the `random_element` function of the polynomial quotient ring
passes all arguments through to the base ring. However, the
random_element function itself passes a default degree to the base ring,
based on the degree of the quotient ring. So, overwriting the degree in
kwargs gives a TypeError. Therefore, detect the `degree` argument
manually, such that the user can overrride the default degree.

<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [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.
- [x] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#37268
Reported by: Emiel Wiedijk
Reviewer(s): Emiel Wiedijk, Sebastian A. Spindler
@vbraun vbraun merged commit 044b3aa into sagemath:develop Feb 25, 2024
14 of 20 checks passed
@aimileus aimileus deleted the pol-quotient-deg branch February 25, 2024 11:34
@mkoeppe mkoeppe added this to the sage-10.3 milestone Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@S17A05 S17A05 S17A05 approved these changes

Assignees
No one assigned
Labels
c: commutative algebra p: minor / 4
Projects
None yet
Milestone
sage-10.3
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@aimileus @S17A05 @vbraun @mkoeppe