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: |
Description (last modified by )
This function uses a formula by Fulton to count points on toric varieties over finite fields.
Apply
Attachments (2)
Change History (16)
Changed 9 years ago by
Attachment: | toric_point_count.patch added |
---|
comment:1 Changed 9 years ago by
Status: | new → needs_review |
---|
comment:2 Changed 9 years ago by
comment:3 Changed 9 years ago by
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
Rewrote examples via change_ring
which is certainly more pleasant than reconstructing varieties.
comment:5 Changed 9 years ago by
I would still be in favor of calling it count_points
to match the elliptic curve code.
Changed 9 years ago by
Attachment: | trac_14891_reviewer.patch added |
---|
comment:6 Changed 9 years ago by
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:8 Changed 9 years ago by
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
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
Status: | needs_review → positive_review |
---|
comment:11 Changed 9 years ago by
Status: | positive_review → needs_work |
---|
This patch conflicts with #12900. One should be rebased on top of the other.
comment:12 Changed 9 years ago by
Status: | needs_work → positive_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
Description: | modified (diff) |
---|
comment:14 Changed 9 years ago by
Merged in: | → sage-5.12.beta1 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
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
The Python convention is to use underscore as method names (like
count_points
) and CamelCase for class names.base_ring()
to get the field over which to count pointsn
for finite fields withp^n
elements, see the elliptic curvecount_points()
method.TypeError
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 withsage -docbuild all html