-
-
Notifications
You must be signed in to change notification settings - Fork 421
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
improve random sampling of quotient-ring elements #37367
improve random sampling of quotient-ring elements #37367
Conversation
Documentation preview for this PR (built with commit 5c1309c; changes) is ready! 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, and even if there's a smarter method this could be done in the future. I think this is good to go.
Currently, random sampling in generic quotient rings is restricted to a very small (and special) subset of the elements: ```sage sage: R.<x,y> = QQ[] sage: S = R.quotient([x^3, y^2]) sage: {S.random_element() for _ in range(999)} {-2, -1, 0, 1, 2} ``` In this patch we add an implementation of `.random_element()` which simply calls the `.random_element()` method of the cover ring and maps the result to the quotient. This is still far from perfect for many kinds of quotient rings, but it's definitely an improvement compared to the current behavior. URL: sagemath#37367 Reported by: Lorenz Panny Reviewer(s): Giacomo Pope
Currently, random sampling in generic quotient rings is restricted to a very small (and special) subset of the elements: ```sage sage: R.<x,y> = QQ[] sage: S = R.quotient([x^3, y^2]) sage: {S.random_element() for _ in range(999)} {-2, -1, 0, 1, 2} ``` In this patch we add an implementation of `.random_element()` which simply calls the `.random_element()` method of the cover ring and maps the result to the quotient. This is still far from perfect for many kinds of quotient rings, but it's definitely an improvement compared to the current behavior. URL: sagemath#37367 Reported by: Lorenz Panny Reviewer(s): Giacomo Pope
It would be good to also pass options along. |
Good point, but it looks like @vbraun is already merging this — is it okay to still change the branch now? |
Hmmm….possibly no. Let’s just make another PR, and cc me for the review. |
Currently, random sampling in generic quotient rings is restricted to a very small (and special) subset of the elements:
In this patch we add an implementation of
.random_element()
which simply calls the.random_element()
method of the cover ring and maps the result to the quotient. This is still far from perfect for many kinds of quotient rings, but it's definitely an improvement compared to the current behavior.