Moving/cleaning enumeration functions for points on schemes

[sagetrac-cturner]

I think it would be sensible to move the functions

enum_projective_rational_field

enum_projective_finite_field

enum_affine_rational_field

enum_affine_finite field

from their current position at the top of sage.schemes.generic.homset, and to clean up their code to make it easier to read whilst (I believe) keeping the timing about the same.

I have started work on a patch that does this, putting them into a new file sage.schemes.generic.rational_point and using the cartesian_product_iterator function to make the code much more readable.

This is a preamble to putting other (more efficient) functions that find rational points in specific cases into the same file - I am currently working on this, and will make a separate ticket for it.

CC: @JohnCremona @wdjoyner

Component: algebraic geometry

Keywords: rational points enumeration

Author: Charlie Turner, David Joyner, Robert Miller, Andrey Novoseltsev

Reviewer: Robert Miller, Andrey Novoseltsev

Merged: sage-4.6.2.alpha0

Issue created by migration from https://trac.sagemath.org/ticket/9055

[sagetrac-cturner]

Changed author from Charlie Turner to none

[novoselt]

comment:2

I support the idea of moving these functions - I was a bit surprised to see them when I was going over homset file. And of course nobody should be against making them more readable ;-)

[sagetrac-cturner]

comment:3

I have uploaded a patch. It does the following:

1. Moves the functions mentioned to a new file, rational_point
2. Changed enum_projective_rational_field, enum_projective_finite_field, enum_affine_finite_field to be, I hope, more readable, but to run no faster. I have also added a sort at the end.
   I did not change enum_affine_rational_field in the end because I couldn't see a simpler way to do it.
3. Added docstrings for these four functions.
4. Changed some doctests for the rational_points function (which use these functions) in the algebraic_scheme file. This is because the old functions returned an unsorted list and the new ones sort before returning, so the ordering has changed but the set of points has not.

[novoselt]

comment:4

I just glanced at the patch, but I have the following comments:

1. "Set of homomorphisms between two schemes" should stay in the original file, it is the docstring of the module describing what does it do.

2. There should be a new description (hopefully, more informative) in the new module. The first line is how this module will be listed in the documentation, the rest can be any text. Also, there should be the copyright/license notice and AUTHORS field in the module docline (stating who has written these functions initially and that you have improved and documented them).

3. There are some doctests with very long output strings. This leads to not very good looking formatting of the documentation. You can insert line breaks in the output and doctests still will run fine. I would suggest doing it to keep line length under 80 (or even 60, excluding leading spaces,) characters.

4. Did you try to compile a documentation with this module? I didn't, but it seems to me that there are quite a few problems with indentations and "::"s. Also, now that you have actually added the docstrings to these functions, it would make sense to include this module into the auto-generated documentation tree.

5. Is there any specific reason for importing these functions in the homset module right before their use? It is my strong belief that all imports should be in the beginning of the file to avoid repetitions and to make it clear what is used by this module.

I think that 1), 2), and 4) are mandatory, while 3) and 5) may reflect my personal taste (although I tried to explain why do I think that they make sense).

[sagetrac-cturner]

comment:5

1. I (believe I have) fixed the issues flagged by novoselt. Thank you novoselt for your help in pointing them out.

2. I have improved the code so that it can take either a scheme or an abstract set of rational points of a scheme as input.

THERE IS A KNOWN DOCTEST FAIL with this patch;

The following tests failed:

sage/coding/code_constructions.py
sage/coding/linear_code.py
sage/coding/decoder.py
doc/en/constructions/linear_codes.rst

I think this is because the coding modules use the enum_projective_finite_field function. This causes the same problem as point 4. of my previous comment (circa 8th June). The new versions of each of these functions sort points but the old ones did not. I suspect that the coding functions depend on the order of the output of enum_projective_finite_field and so fail now that that order has changed. I am not proficient with codes and do not feel able to fix this issue. I wonder if someone who does understand coding could help and fix this? I CC David Joyner, who originally wrote these modules. Perhaps it is simply a matter of adapting the doctests to take into account the new ordering. I believe that this is the right way round; that the ordering of the output of the "enum" functions should not depend on the algorithm used.

Because of this, I suggest that the patch be reviewed ignoring these problems with coding modules, and in the meantime a coding person might be able to help solve the problems.