Opened 10 years ago
Closed 10 years ago
#11166 closed defect (fixed)
Typo in initialization of FreeModule_generic_field
Reported by: | nborie | Owned by: | nborie |
---|---|---|---|
Priority: | major | Milestone: | sage-4.7.2 |
Component: | linear algebra | Keywords: | |
Cc: | Merged in: | sage-4.7.2.alpha2 | |
Authors: | Nicolas Borie | Reviewers: | Kelvin Li, Rob Beezer |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
base_field
became base_ring
... it is very funny this bug never made things go bad.
sage: from sage.modules.free_module import FreeModule_generic_field sage: FreeModule_generic_field(QQ, 5, 5) Traceback (most recent call last): ... NameError: global name 'base_ring' is not defined
And in the code:
class FreeModule_generic_field(FreeModule_generic_pid): ... def __init__(self, base_field, dimension, degree, sparse=False): ... FreeModule_generic_pid.__init__(self, base_ring, dimension, degree, sparse=sparse)
As the creation is done with a factory, this bug is invisible from the user. But, if you want to build your own FreeModule
over a field with category and some other stuff, calling the __init__
of this class make me fall on the bug.
Apply:
Attachments (5)
Change History (16)
Changed 10 years ago by
comment:1 Changed 10 years ago by
- Status changed from new to needs_review
comment:2 Changed 10 years ago by
- Description modified (diff)
Patch looks good and passes sage --testall --long
on 4.7. I think all that is missing is a doctest that directly exercises this function. Someone needs to carefully check whether my attempt at a doctest is sufficient.
comment:3 Changed 10 years ago by
- Description modified (diff)
- Reviewers set to Kelvin Li
comment:4 follow-up: ↓ 5 Changed 10 years ago by
Please correct the headers, first line should contain exactly #11166
Apply trac_11166_FreeModule_generic_field-nb-v2.patch, trac_11166_doctest.patch
comment:5 in reply to: ↑ 4 Changed 10 years ago by
Replying to chapoton:
Please correct the headers, first line should contain exactly #11166
Sorry, I do not understand what you mean. Are you referring to the commit messages in the patches?
comment:6 follow-up: ↓ 7 Changed 10 years ago by
Yes, sorry for being unclear. The patch buildbot apparently requires that the first line of the commit message should contain the ticket number, in the specific format #xxxxx, with the # attached before the number. Otherwise, the bot will not give a green report. I was trying to help this ticket to reach the green report.
comment:7 in reply to: ↑ 6 Changed 10 years ago by
Replying to chapoton:
Yes, sorry for being unclear. The patch buildbot apparently requires that the first line of the commit message should contain the ticket number, in the specific format #xxxxx, with the # attached before the number. Otherwise, the bot will not give a green report. I was trying to help this ticket to reach the green report.
Thanks for the explanation; I have updated the affected patch. I've known about the patchbot for a while, but I can't find any documentation on it. I don't even know how it determines which patches to apply!
comment:8 Changed 10 years ago by
- Reviewers changed from Kelvin Li to Kelvin Li, Rob Beezer
- Status changed from needs_review to positive_review
This looks all ready to go. Passes long tests on 4.7.1.alpha4. So positive review.
Quite amazing this didn't crop up sooner!
I think both patches need their commit string edited, I am going to put up corrected versions.
Rob
Changed 10 years ago by
Changed 10 years ago by
comment:9 Changed 10 years ago by
- Description modified (diff)
comment:10 Changed 10 years ago by
- Milestone changed from sage-4.7.1 to sage-4.7.2
comment:11 Changed 10 years ago by
- Merged in set to sage-4.7.2.alpha2
- Resolution set to fixed
- Status changed from positive_review to closed
Will see what the builbot will say...
This fixed :