Ticket #2480 (closed defect: fixed)

Opened 5 years ago

Last modified 3 years ago

problem parsing arguments to NumberField.order()

Reported by: ncalexan Owned by: davidloeffler
Priority: minor Milestone: sage-4.3.2
Component: number fields Keywords: number field order arguments
Cc: ncalexan, robertwb, mhansen Work issues:
Report Upstream: N/A Reviewers: Mike Hansen
Authors: Craig Citro Merged in: sage-4.3.2.alpha0
Dependencies: Stopgaps:

Description

sage: y = ZZ['y'].0; K = NumberField(y^4 + 4*y^2 + 2, 'a'); K
Number Field in a with defining polynomial y^4 + 4*y^2 + 2
sage: B = K.integral_basis()
sage: B
[1, a, a^2, a^3]
sage: K.order(B)
Order in Number Field in a with defining polynomial y^4 + 4*y^2 + 2
sage: K.order(gens=B)
+Infinity

Attachments

trac_2480.patch Download (2.0 KB) - added by craigcitro 3 years ago.

Change History

comment:1 Changed 4 years ago by davidloeffler

  • Owner changed from was to davidloeffler
  • Component changed from number theory to number fields

comment:2 Changed 3 years ago by craigcitro

  • Status changed from new to needs_review
  • Report Upstream set to N/A
  • Authors set to Craig Citro

This wasn't so bad -- the problem was that gens= put the list of gens in the kwds dict, instead of in the *-argument. I've attached a fix, but I'd love for someone to tell me if deleting gens out of the kwds dict is sufficiently pythonic. (If you don't, the call to absolute_order_from_ring_generators rightfully complains that gens is specified twice.) Another option would be to reassign kwds['dict'] at the end, but I don't think that's any nicer. (In fact, that might be epsilon slower, since it's another argument to unpack from the dictionary on the other side.)

Also, the comment block in the docstring really looks like something was accidentally cut off at some point. Amusingly, this isn't the case: I actually dug through the hg logs, and it was really committed just like that.

comment:3 Changed 3 years ago by craigcitro

  • Cc robertwb, mhansen added

Mike and Robert, I'm adding you on the cc so that you can tell me if I'm being sufficiently pythonic. :)

comment:4 Changed 3 years ago by mhansen

Hey Craig,

gens = kwds.pop('gens')

is probably better.

comment:5 Changed 3 years ago by mhansen

Err,

gens = kwds.pop('gens', args)

Changed 3 years ago by craigcitro

comment:6 Changed 3 years ago by craigcitro

Nice. New patch with Mike's suggestion incorporated posted.

comment:7 Changed 3 years ago by mhansen

  • Status changed from needs_review to positive_review
  • Reviewers set to Mike Hansen

comment:8 Changed 3 years ago by mvngu

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