Ring support for projective space points and morphisms

[bhutz]

Projective points and morphisms are currently only support over fields. Support for rings is needed.

Apply:

• trac_13130_rings_for_projective_space.patch

[bhutz]

Summary: Adding support for rings in projective spaces.

See comments from:  https://groups.google.com/forum/?fromgroups#!topic/sage-devel/HXXqQ_XP358

point.py:

-no automatic normalization (as is currently done with fields)

-implemented eq and ne, added a note in cmp for ell_point.py that eq and ne are implemented for projective points

-implemented scale_by() - scales the coordinates by a given amount

-implemented normalize_coordinates() - removes the gcd of the coordinates

morphism.py:

-no automatic gcd removal (even over CC this does not work) This change reverses part of Trac: 3964 ( http://trac.sagemath.org/sage_trac/ticket/3964)

-removed "immutable" for .polys since you can scale projective coordinates

-added "check" to call to help deal with indeterminacies and allow for bypassing the initialization checks for points

-implemented scale_by() - scales the coordinates by a given amount

-implemented normalize_coordinates() - removes the gcd of the coordinates

homset.py:

-allowed rings

projective_space.py:

-implemented dimension() - since the current implementation was giving incorrect values when the base ring is a ring (for example ZZ)

general changes:

-added many examples

-cleaned up imports

[bhutz]

Updated so that it applies on Sage-5.1

[annahaensch]

Replaces trac_13130_rings_for_projective_space.patch

[annahaensch]

I ran -t on each of the following files:

• schemes/elliptic_curves/ell_point.py
• schemes/generic/homset.py
• schemes/generic/morphism.py
• schemes/generic/point.py
• schemes/generic/projective_space.py
• schemes/readme.py

and "All tests passed!"

I ran -coverage to check the doctest coverage, all affected files had 100% coverage, except:

• schemes/generic/morphism.py (95%)
• schemes/generic/point.py (37%)

I brought morphism.py to 100%, but point.py is still missing things.

I ran --testall --long and got the following error:

******************************
File "/Users/annahaensch/sage-5.2/devel/sage/sage/interfaces/r.py", line 1169:
    sage: os.path.realpath(tmpdir) == sageobj(r.getwd())
Expected:
    True
Got:
    False
******************************************
1 items had failures:
   1 of   7 in __main__.example_43
Test Failed**** 1 failures.

Just to be sure, I ran a longtest just on interfaces/r.py, and all tests passed, which is unsurprising, since this patch doesn't touch that file…not sure what to do about that one.

I ran -docbuild reference html and got one LaTex? error, line 531 of sage/schemes/generic/projective_space.py. Changed `\PP^N` to '\mathbb{P}^N` .

[novoselt]

What is the reason for needs_work?

More importantly - what is the reason for replacing the original patch? Do you propose a different implementation? Why? What are the differences?

[annahaensch]

As I describe in the comment above, the new patch reflects some minor changes to bring doctest coverage up to 100% and fix the docbuild errors. Also, mentioned above, it still fails to pass the long test, due to some errors in r functionality, which is why the patch needs_work.

[bhutz]

Thanks for reviewing this Anna (and the doc fix). Just to be sure I understand the current situation:

when you run --testall --long you get an error in r.py, but when you run -t --long on r.py you get all passed?

[annahaensch]

Replying to bhutz:

Thanks for reviewing this Anna (and the doc fix). Just to be sure I understand the current situation:

when you run --testall --long you get an error in r.py, but when you run -t --long on r.py you get all passed?

That's correct. The patch must call some routines from r.py, but not actually touch r.py.

[novoselt]

If the new patch reflect some minor changes, it should be a small reviewer patch on top of the original (quite big) one. If the original patch cannot be applied anymore at all, that's another issue.

It is also not clear if failures in r.py are related to this ticket. Do you always get an error with --testall when this patch is installed and never without it?

[annahaensch]

Replying to novoselt:

If the new patch reflect some minor changes, it should be a small reviewer patch on top of the original (quite big) one. If the original patch cannot be applied anymore at all, that's another issue.

I wasn't sure what the convention was for fixing patches. If that's easier, I can certainly upload a new patch to reflect that convention.

It is also not clear if failures in r.py are related to this ticket. Do you always get an error with --testall when this patch is installed and never without it?

That's correct. Before testing this patch I built sage-5.2 from source and ran --testall --long, and all tests passed. I applied the patch, re-ran the test, and got the error.

[novoselt]

Replying to annahaensch:

Replying to novoselt:

If the new patch reflect some minor changes, it should be a small reviewer patch on top of the original (quite big) one. If the original patch cannot be applied anymore at all, that's another issue.

I wasn't sure what the convention was for fixing patches. If that's easier, I can certainly upload a new patch to reflect that convention.

Please do ;-)

It is also not clear if failures in r.py are related to this ticket. Do you always get an error with --testall when this patch is installed and never without it?

That's correct. Before testing this patch I built sage-5.2 from source and ran --testall --long, and all tests passed. I applied the patch, re-ran the test, and got the error.

So - you ran tests ONCE without the patch and there were no error and then ONCE with the patch and there was one? If that is the case and the error does not appear when testing that file separately, it is still quite that you hit a sporadic bug which is not relates to this ticket (and I am pretty sure I've seen recently other complains on this module). Can you test with the patch and --testall --long again several times and see if it is always the same failure?

[novoselt]

For the original patch:

• NOTES:: should be formatted as .. note::
• Names of methods, types, etc. should be at the very least double quoted like ``__eq__`` or better yet referred to like :class:`ValueError`
• Why _element_constructor_ was removed? It is part of the coercion framework and it is likely that it has to be present.
• It looks to me like the code of class SchemeMorphism_polynomial_projective_space was moved inside the same file. Why? Was it changed?
• Input/output is not documented everywhere and formatting is not always correct. See http://www.sagemath.org/doc/developer/conventions.html#documentation-strings
• dimension method is defined to just return relative_dimension Why??? dimension does work already and works differently:

  sage: ProjectiveSpace(3,QQ["y"],'x').dimension_relative()
  3
  sage: ProjectiveSpace(3,QQ["y"],'x').dimension()
  4
  

  The patch breaks backward compatibility and makes dimension inconsistent between different spaces.

[bhutz]

as another data point the --testall --long passes on my machine (Ubuntu)

To answer the questions in the last comment:

The formatting I can address. I had thought I'd followed the conventions, but I'll go back through and check all the changes.

_element_constructor was removed since it was simply returning NotImplemented?. As in points_projective_field, removing that place holder allows the SchemeHomset?_points _element_constructor to be used for point_projective_ring

SchemeMorphism?_polynomial_projective_space was moved to have the projective classes together. If it is more standard in Sage to have the _polynomial_ classes together, then I can certainly move it back.

Dimension was changed since it seemed to be not working for projective spaces. Before the new dimension function you would get

sage: ProjectiveSpace?(QQ,2,'x').dimension()

2

sage: ProjectiveSpace?(ZZ,2,'x').dimension()

3

I assumed that the value for ZZ was wrong. Although it seems that Spec(ZZ).dimension() does in fact return 1 while Spec(QQ).dimension() returns 0. Is this really the desired behavior, that the dimension of projective space over the integers is +1?

[novoselt]

It is standard in Sage to keep track of who is doing what and why, if you rearrange a file, then you will be shown as the author of new lines and this ticket will be listed as a reason for its addition. There can be different schemes of arranging names and classes (e.g. I like to have everything alphabetically). I don't think we have any standard conventions for that. But you should never "just move" existing code because of the history loss.

Integers have a chain of prime ideals of length 1, e.g. (0) contained in (2), so its dimension is one. Rationals have dimension zero since there is only one prime ideal (length of the chain is zero), as for other fields.

If you look at the documentation of existing dimension function, you will see that it is indeed supposed to work like this: dimension of the base ring is taken into consideration. Moreover, you can see that there are long names for either way dimension_absolute and dimension_relative, and just dimension is explicitly shown to work as dimension_absolute. So it is definitely not a good idea to make this "default" dimension mean relative in the case of projective spaces.

[bhutz]

I've made the following changes:

• found and fixed the formatting
• included the docstring fix and the coverage fix from Anna's version
• no longer implement a dimension function
• returned the :class:SchemeMorphism_polynomial_projective_space to its original location

testall passes on my machine.

However, I'm not sure what the right step for the file attachment is. I can replace my original patch with my new one, but cannot replace or remove Anna's version. Should I replace my original version or upload it as a new (3rd) version?

[novoselt]

Usually it is OK to replace your patch with an updated one. When there is more than one patch on the ticket, it is also a good idea to include "Apply" section in the description, see e.g. #11763. This is for reviewers and the release manager, who should not go through all the comments once this ticket is ready to be merged. You can also add a comment "Apply ..." for the patchbot, but it is moody and does not always pick it up.

[bhutz]

I went to check this on 5.3 and noticed that I had implemented scale_by() and normalize_coordinates() in class::SchemeMorphism?_polynomial. That was wrong since they should only apply to projective maps. So I've moved those to SchemeMorphism?_polynomial_projective_space.

They modify the _ _polys attribute of the SchememMorphism?_polynomial, but a _ _ attribute is private. Since it makes sense to inherit the polys attribute, I changed it to ._polys. I checked through the source and found no place where that created an issue and all tests currently pass.

Also, I noticed that somehow I've created a bunch of

_ 'blank line'

+ 'blank line'

I tried to removed those directly from the patch file and wrecked patch. If someone could tell me how to get those out of there, I'd like to remove them.

[bhutz]

Updated to work with Sage 5.5.

[bhutz]

While working on something else, I found a bug in the patch. It's been fixed and the new patch uploaded.

[vbraun]

Patch needs to be rediffed for current sage...

[bhutz]

All seems good on 5.7

[vbraun]

Patchbot: apply trac_13130_rings_for_projective_space.patch

[bhutz]

Minor changes from review.

[bhutz]

minor changes from review

[mmanes]

I have run longtest, done docbuild, and checked for coverage. Everything passes 100%.

I have also tested functionality pretty extensively, and everything works great.

This patch should definitely be incorporate because it is the foundation that will allow future development of arithmetic dynamics functionality.

[chapoton]

Well, this is closed, but you still need to correct the doc : there are wrong "INPUT::" everywhere.

[jdemeyer]

I agree, the documentation formatting is horrible.

[bhutz]

Ok. This will be fixed as part of #14217.