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:

Status badges

Description (last modified by rbeezer)

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:

  1. trac_11166_FreeModule_generic_field-nb-v3.patch
  2. trac_11166_doctest-v2.patch

Attachments (5)

trac_11166_FreeModule_generic_field-nb.patch (837 bytes) - added by nborie 10 years ago.
trac_11166_doctest.patch (1.1 KB) - added by ltw 10 years ago.
attempt at a doctest
trac_11166_FreeModule_generic_field-nb-v2.patch (853 bytes) - added by ltw 10 years ago.
rebased on 4.7
trac_11166_FreeModule_generic_field-nb-v3.patch (792 bytes) - added by rbeezer 10 years ago.
trac_11166_doctest-v2.patch (1017 bytes) - added by rbeezer 10 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 10 years ago by nborie

  • Status changed from new to needs_review

Will see what the builbot will say...

This fixed :

sage: from sage.modules.free_module import FreeModule_generic_field
sage: FreeModule_generic_field(QQ, 5, 5)
<class 'sage.modules.free_module.FreeModule_generic_field_with_category'>

Changed 10 years ago by ltw

attempt at a doctest

comment:2 Changed 10 years ago by ltw

  • 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 ltw

  • Authors set to Nicolas Borie
  • Description modified (diff)
  • Reviewers set to Kelvin Li

comment:4 follow-up: Changed 10 years ago by chapoton

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 ltw

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: Changed 10 years ago by 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.

Changed 10 years ago by ltw

rebased on 4.7

comment:7 in reply to: ↑ 6 Changed 10 years ago by ltw

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 rbeezer

  • 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 rbeezer

comment:9 Changed 10 years ago by rbeezer

  • Description modified (diff)

comment:10 Changed 10 years ago by jdemeyer

  • Milestone changed from sage-4.7.1 to sage-4.7.2

comment:11 Changed 10 years ago by jdemeyer

  • Merged in set to sage-4.7.2.alpha2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.