Skip to content

Raise exception when transformation not supported for echelon_form() #39981

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

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

HenryWu2101
Copy link
Contributor

@HenryWu2101 HenryWu2101 commented Apr 20, 2025

#37480
This is a preliminary fix for the issue mentioned, where it raises exception whenever echelon_form() does not support argument "transformation=True".
The list of all matrix class that has its own echelon_form():
matrix2
matrix_cyclo_dense
matrix_integer_dense
matrix_mpolynomial_dense
matrix_rational_dense
matrix_rational_sparse

The list of matrix class that does not support transformation and thus modified to raise exception:
matrix_cyclo_dense
matrix_mpolynomial_dense.pyx
matrix_rational_dense.pyx
matrix_rational_sparse.pyx

We should for the moment use this fix for the issue so that we don't have the "ignore user input" phenomenon, but keep the issue for future.

📝 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 and checked the documentation preview.

⌛ Dependencies

cc @vneiger @maxale

@HenryWu2101 HenryWu2101 changed the title Except Raise exception when transformation not supported for echelon_form() Apr 20, 2025
Copy link

github-actions bot commented Apr 20, 2025

Documentation preview for this PR (built with commit b08c7c6; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

This is more aligned with the previous behavior and more suitable for a change in transition, where it still gives output but along with a message indicating a workaround for the user side.
@HenryWu2101
Copy link
Contributor Author

@vneiger @maxale For the latest commit, I have changed the "raise exception" approach to "warnings", which I feel would be more aligned with the previous behavior, when this time it gives a result for echelon_form() and with a message suggesting a workaroud. This I think is a better solution for a temporary fix.

@vneiger
Copy link
Contributor

vneiger commented Apr 22, 2025

Thanks for the work. What about other classes, e.g. matrix_mod2_dense, matrix_modn_dense, matrix_polynomial_dense, ... ? I do not see where they are handled in your changes. Apart from that, I tried your code on the modified methods, and the warning idea looks good to me.

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.

2 participants