Opened 9 years ago
Closed 7 years ago
#3893 closed enhancement (fixed)
random_element methods should all accept *args and **kwds
Reported by: | malb | Owned by: | somebody |
---|---|---|---|
Priority: | major | Milestone: | sage-4.6 |
Component: | basic arithmetic | Keywords: | random_element, args, kwds |
Cc: | Merged in: | sage-4.6.alpha3 | |
Authors: | Niles Johnson | Reviewers: | Martin Albrecht |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
Because:
- some more highlevel
random_element
functions pass thru*args
and**kwds
- it unifies the interface to some extend
- it allows the user to pass thru parameters
Apply
Attachments (5)
Change History (17)
comment:1 Changed 7 years ago by
- Report Upstream set to N/A
- Status changed from new to needs_review
Changed 7 years ago by
Changed 7 years ago by
comment:2 Changed 7 years ago by
I rebased the patch to 4.5.3.alpha2 and ran long doctests: all passed. The patch also looks good, the only issue I'd have: I'd either move the lines "Trac#3893 ..." to the TESTS:: section or remove that line completely. I understand that people want to point out where something changed, but that docstring is saying nothing to a user since it is developer information.
comment:3 follow-up: ↓ 4 Changed 7 years ago by
Thanks for taking a look!
Somewhere I gained the impression that doctests for a ticket should mention the ticket number--is that not necessarily the case? I don't really have a strong opinion, but since it may be non-obvious functionality I'm in favor of keeping these tests in the EXAMPLES:: section. How about rewording the description to something like the following?
Passes extra positional or keyword arguments through (Trac #3893)::
comment:4 in reply to: ↑ 3 ; follow-up: ↓ 5 Changed 7 years ago by
- Reviewers set to Martin Albrecht
Replying to niles:
Thanks for taking a look! Somewhere I gained the impression that doctests for a ticket should mention the ticket number--is that not necessarily the case? I don't really have a strong opinion, but since it may be non-obvious functionality I'm in favor of keeping these tests in the EXAMPLES:: section. How about rewording the description to something like the following?
Passes extra positional or keyword arguments through (Trac #3893)::
I'm not sure what the official policy is, but I'd say it would be a bit odd when in a few years time the docstring is cluttered with trac ticket numbers, what information do they communicate that is relevant? I am very much in favour of having "Passes extra positional or keyword arguments through::
". I leave it up to you, so you have a positive review, i.e. toggle it yourself if you don't want to change it.
comment:5 in reply to: ↑ 4 Changed 7 years ago by
Replying to malb:
I'm not sure what the official policy is, but I'd say it would be a bit odd when in a few years time the docstring is cluttered with trac ticket numbers
fair enough; I also can't find any mention of a policy like this in the developer guide, so I've changed the docstring as you suggested. Since I don't have 4.5.3a2 installed, will you re-verify that the patch applies cleanly before hitting "positive review"? (Since the changes were easy, I just made them directly to the rebased patch file itself.)
comment:6 Changed 7 years ago by
- Status changed from needs_review to positive_review
rebased patch applies cleanly to 4.5.3; switching to positive review, as per Martin's recommendation
comment:7 Changed 7 years ago by
With the latest patch applied to a trial 4.6.alpha2, I get two doctest failures:
sage -t -long "devel/sage/sage/rings/contfrac.py" ********************************************************************** File "/mnt/usb1/scratch/mpatel/tmp/sage-4.6.alpha1-working/devel/sage/sage/rings/contfrac.py", line 40: sage: a Expected: [ [-1, 2] [-1, 1, 94] [0, 2] [-12]] [ [-1] [0, 2] [-1, 1, 3] [0, 1, 2]] [ [-3, 2] [0, 1, 2] [-1] [1]] [ [-1] [0, 3] [1] [-1]] Got: [ [-1, 2] [-1, 1, 94] [0, 2] [-12]] [ [-1] [0, 2] [-1, 1, 3] [0, 1, 2]] [ [-3, 2] [0] [0, 1, 2] [-1]] [ [1] [-1] [0, 3] [1]] ********************************************************************** File "/mnt/usb1/scratch/mpatel/tmp/sage-4.6.alpha1-working/devel/sage/sage/rings/contfrac.py", line 46: sage: f Expected: [1]*x^4 + [2]*x^3 + ([-12, 1, 14, 7, 1, 1, 7])*x^2 + ([-40, 2, 32, 2, 1, 1, 2])*x + [8, 1, 5, 2, 14, 1, 1, 3] Got: [1]*x^4 + ([-2, 3])*x^3 + [14, 1, 1, 1, 9, 1, 8]*x^2 + ([-13, 4, 1, 2, 1, 1, 1, 1, 1, 2, 2])*x + [-6, 1, 5, 9, 1, 5] **********************************************************************
Perhaps there's an interaction with one of the already merged tickets?
comment:8 Changed 7 years ago by
- Status changed from positive_review to needs_info
comment:9 Changed 7 years ago by
- Description modified (diff)
- Keywords random_element args kwds added
- Status changed from needs_info to needs_review
comment:10 Changed 7 years ago by
Martin, could you review the latest patch? If it's OK, I'll merge it into 4.6.alpha3.
comment:11 Changed 7 years ago by
- Status changed from needs_review to positive_review
The updated patch applies with some fuzz, but it applies. Doctests pass. I trust only doctest failures were addressed and thus I give this a positive review.
comment:12 Changed 7 years ago by
- Merged in set to sage-4.6.alpha3
- Resolution set to fixed
- Status changed from positive_review to closed
I've reinterpreted this ticket as "random_element methods which call other random_element methods should pass on *args and kwds". This applies mostly to
sage.rings
, but also a few other cases. The list below shows the output ofsage: search_src("def random_element")
, annotated by the changes this patch makes.make ptestlong
passes all tests, and documentation builds without errors or warnings. Details below.Note that #9481 fixes
random_element
for univariate power series rings, and I've ignored padics since there is a large revision in process: e.g. #6084 and #5075.