Skip to content

fixed bug in is_linearly_dependent(), returns true for linearly dependent univariate polynomial. #37174

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
Merged
merged 3 commits into from
Feb 2, 2024
Merged

fixed bug in is_linearly_dependent(), returns true for linearly dependent univariate polynomial. #37174

merged 3 commits into from
Feb 2, 2024

Conversation

amanmoon
Copy link
Contributor

@amanmoon amanmoon commented Jan 27, 2024 •
edited by tscrim
Loading

is_linearly_dependent() returned wrong results for the univariate polynomial ring because the coefficients() method of univariate polynomials returns coefficients in the opposite order of monomials():

sage: R.<q> = QQ[]
sage: p = q^4 + 3*q + 5
sage: p.monomials()
[q^4, q, 1]
sage: p.coefficients()
[5, 3, 1]

We fix this by making the coefficient_matrix() method to check for univariate polynomial ring and make the order consistent.

Fixes: #37075

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

@amanmoon amanmoon changed the title is_linearly_dependent() returns true for linearly dependent invariant polynomial. fixed bug in is_linearly_dependent(), returns true for linearly dependent invariant polynomial. Jan 28, 2024
@amanmoon amanmoon changed the title fixed bug in is_linearly_dependent(), returns true for linearly dependent invariant polynomial. fixed bug in is_linearly_dependent(), returns true for linearly dependent univariate polynomial. Jan 28, 2024
tscrim
tscrim requested changes Jan 29, 2024
View reviewed changes
Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

I don’t think we should add additional arguments to coefficients(). It is good to clarify the specification that it is by decreasing degree (with the better doctests). However, the user can simply reverse the output if they want it in the opposite order. We don’t need to make our code more complicated for a 1-line common Python operation (as can be seen my the suggested change).

@amanmoon amanmoon force-pushed the bug/is_linearly_dependent branch from b6c38a9 to 6fbb720 Compare January 29, 2024 16:07
@amanmoon amanmoon requested a review from tscrim January 29, 2024 16:12
@github-actions GitHub Actions
Copy link

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

tscrim
tscrim approved these changes Jan 30, 2024
View reviewed changes
Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

Thank you.

@vbraun vbraun merged commit 9b131cd into sagemath:develop Feb 2, 2024
@amanmoon amanmoon deleted the bug/is_linearly_dependent branch February 3, 2024 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tscrim tscrim tscrim approved these changes

Assignees
No one assigned
Labels
c: algebra t: bug
Projects
None yet
Milestone
sage-10.3
Development

Successfully merging this pull request may close these issues.

Method is_linearly_dependent() is broken
4 participants
@amanmoon @tscrim @vbraun @mkoeppe