-
Notifications
You must be signed in to change notification settings - Fork 73
Simplify rand for UniversalPolyRing
#2257
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
base: master
Are you sure you want to change the base?
Conversation
|
I mention the following (rather aged) PR for Oscar: oscar-system/Oscar.jl#4787 |
4abe836 to
f191f1e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2257 +/- ##
==========================================
- Coverage 88.07% 88.04% -0.04%
==========================================
Files 126 126
Lines 31728 31715 -13
==========================================
- Hits 27946 27925 -21
- Misses 3782 3790 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lgoettgens
left a comment
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.
This removes the possibility to create a RandomExtensions.jl Sampler object for a universal polynomial ring (on which all of the current random generation is based on). See #1388 for more context (even though the people there disagree with me, I think this should be coordinated with the other parts of AA that rely on RandomExtensions.jl).
If you still want to proceed with something like this here, you could keep the sampler generation code, and there just wrap a sampler of the underlying mpoly ring and delegate all the further rand calls to that sampler
I think that was @SoongNoonien's intention, but it's difficult to do given the lack of tests and documentation. Plus we do some really funky stuff -- there is a reason why my PR Nemocas/Nemo.jl#1819 has been stalled for 1.5 years: I couldn't figure out how to interact with the RandomExtensions APIs in a way that kept the existing code working without running into invalidations 🤷 As I see it: who needs "samplers" for universal polynomial rings? I am only aware of two people using those at all (@SoongNoonien is one of them), and they certainly are not using those. I'd much rather have us implement and move to the new proposed rand(om) APIs (see the PR linked to by @JohnAAbbott) and then ditch the entire Anyway: we'll have a look to see if it is realistic to keep the stuff here working instead of removing it; but that'll require being able to write up some test code for it... |
|
!WARNING! we have several "threads" about random object generation, but there seems to be no "index" of them. Other more-or-less related links are:
If there are missing links, perhaps someone could add them to the list. |
I'm not sure if this makes sense but as far as I understand the random interface this should work. This should just delegate the random element generation to the base ring of the universal polynomial ring.