Opened 9 years ago

Closed 9 years ago

#14891 closed enhancement (fixed)

Counting points on a toric variety over a finite field

Reported by: Adriana Salerno Owned by: mhampton
Priority: minor Milestone: sage-5.12
Component: geometry Keywords: toric varieties
Cc: Merged in: sage-5.12.beta1
Authors: Beth Malmskog, Adriana Salerno, Yiwei She, Christelle Vincent, Ursula Whitcher Reviewers: Volker Braun, Andrey Novoseltsev
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Volker Braun)

This function uses a formula by Fulton to count points on toric varieties over finite fields.

Apply

Attachments (2)

toric_point_count.patch (2.4 KB) - added by Adriana Salerno 9 years ago.
trac_14891_reviewer.patch (4.7 KB) - added by Andrey Novoseltsev 9 years ago.

Download all attachments as: .zip

Change History (16)

Changed 9 years ago by Adriana Salerno

Attachment: toric_point_count.patch added

comment:1 Changed 9 years ago by Adriana Salerno

Status: newneeds_review

comment:2 Changed 9 years ago by Volker Braun

Thanks, looks solid. I have a few implementation suggestions to harmonize it with the way points are counted on elliptic curves. There, you'd write

sage: E = EllipticCurve([3,2])
sage: E.change_ring(GF(11)).count_points()
13

The Python convention is to use underscore as method names (like count_points) and CamelCase for class names.

  • Use the current base_ring() to get the field over which to count points
  • Maybe take an optional integer argument n for finite fields with p^n elements, see the elliptic curve count_points() method.
  • If the base ring is not finite, then raise a TypeError
  • If the toric variety is not smooth, raise a NotImplementedError since one can presumably implement it later.

Also, the docstring should start with a short title like "Return the number of points" before any explanatory sentences. You can use dollar signs or backticks for math (like $F_q$), try building the documentation with sage -docbuild all html

comment:3 Changed 9 years ago by Andrey Novoseltsev

Reviewers: Volker Braun, Andrey Novoseltsev

Missed Volker's comment before writing a patch over which goes more or less along the same lines, but returns infinity for infinite rings. I don't think there is any need for extra parameters, p^n finite fields also can be specified as base ones and for varieties constructed from the same fan there will be no overhead.

Also, it is always a good idea to check how your documentation looks like after compiling: the original patch does not treat references and code blocks properly ;-)

comment:4 Changed 9 years ago by Andrey Novoseltsev

Rewrote examples via change_ring which is certainly more pleasant than reconstructing varieties.

comment:5 Changed 9 years ago by Volker Braun

I would still be in favor of calling it count_points to match the elliptic curve code.

Changed 9 years ago by Andrey Novoseltsev

Attachment: trac_14891_reviewer.patch added

comment:6 Changed 9 years ago by Andrey Novoseltsev

OK, count_points now, although I think that npoints or something similar is more natural and there are plenty of methods that start with n... or n_... ;-)

comment:7 Changed 9 years ago by Beth Malmskog

Wow, thanks for doing all that. Looks great.

comment:8 in reply to:  7 Changed 9 years ago by Andrey Novoseltsev

Replying to malmskog:

Wow, thanks for doing all that. Looks great.

Meaning you agree with changes and want to set it to "positive review"?

Please also take a look at what is changed and why (for why http://sagemath.org/doc/developer/conventions.html is a good reference together with links to PEP8 and PEP257). In addition to Volker's comments and the fact that documentation did not have correct syntax for compiling, there were too long lines (over 79 characters), inconsistent whitespacing, and creating variables for stuff that is used only once to get further stuff:

fan = self.fan() 
cones=fan.cones()

has no advantage over

cones = self.fan().cones()

but is longer, and if cones are used only once, even better to plug self.fan().cones() directly there. (I may have overdone it with folding into 2 lines only, but original 10 were a bit over verbose.)

comment:9 Changed 9 years ago by Beth Malmskog

We do agree with your changes and take your point on the formatting and programming best practices. Of course we think it should have a positive review :). Thanks for working on this.

comment:10 Changed 9 years ago by Andrey Novoseltsev

Status: needs_reviewpositive_review

comment:11 Changed 9 years ago by Jeroen Demeyer

Status: positive_reviewneeds_work

This patch conflicts with #12900. One should be rebased on top of the other.

comment:12 Changed 9 years ago by Frédéric Chapoton

Status: needs_workpositive_review

I have made the other ticket (#12900) depend on this one. Back to positive review for this one.

comment:13 Changed 9 years ago by Volker Braun

Description: modified (diff)

comment:14 Changed 9 years ago by Jeroen Demeyer

Merged in: sage-5.12.beta1
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.