Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#13130 closed enhancement (fixed)

Ring support for projective space points and morphisms

Reported by: Ben Hutz Owned by: Ben Hutz
Priority: major Milestone: sage-5.8
Component: algebraic geometry Keywords: projective space ring
Cc: Moritz Minzlaff Merged in: sage-5.8.beta3
Authors: Ben Hutz Reviewers: Michelle Manes
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Ben Hutz)

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

Apply:

Attachments (2)

trac_13130_rings_for_projective_space_v2.patch (47.7 KB) - added by Anna Haensch 10 years ago.
Replaces trac_13130_rings_for_projective_space.patch
trac_13130_rings_for_projective_space.patch (41.8 KB) - added by Ben Hutz 10 years ago.
minor changes from review

Download all attachments as: .zip

Change History (36)

comment:1 Changed 10 years ago by Ben Hutz

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

Last edited 10 years ago by Ben Hutz (previous) (diff)

comment:2 Changed 10 years ago by Ben Hutz

Status: newneeds_review

comment:3 Changed 10 years ago by Andrey Novoseltsev

Authors: bhutzBen Hutz

comment:4 Changed 10 years ago by Ben Hutz

Updated so that it applies on Sage-5.1

Changed 10 years ago by Anna Haensch

Replaces trac_13130_rings_for_projective_space.patch

comment:5 Changed 10 years ago by Anna Haensch

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` .

comment:6 Changed 10 years ago by Anna Haensch

Status: needs_reviewneeds_work

comment:7 Changed 10 years ago by Andrey Novoseltsev

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?

comment:8 Changed 10 years ago by Anna Haensch

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.

comment:9 Changed 10 years ago by Ben Hutz

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?

comment:10 in reply to:  9 Changed 10 years ago by Anna Haensch

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.

comment:11 Changed 10 years ago by Andrey Novoseltsev

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?

comment:12 in reply to:  11 ; Changed 10 years ago by Anna Haensch

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.

comment:13 in reply to:  12 Changed 10 years ago by Andrey Novoseltsev

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?

comment:14 Changed 10 years ago by Andrey Novoseltsev

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.

comment:15 Changed 10 years ago by Ben Hutz

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?

comment:16 Changed 10 years ago by Andrey Novoseltsev

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.

comment:17 Changed 10 years ago by Ben Hutz

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?

comment:18 Changed 10 years ago by Andrey Novoseltsev

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.

comment:19 Changed 10 years ago by Ben Hutz

Description: modified (diff)

comment:20 Changed 10 years ago by Ben Hutz

Status: needs_workneeds_review

comment:21 Changed 10 years ago by Moritz Minzlaff

Cc: Moritz Minzlaff added

comment:22 Changed 10 years ago by Ben Hutz

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.

comment:23 Changed 10 years ago by Ben Hutz

Updated to work with Sage 5.5.

comment:24 Changed 10 years ago by Ben Hutz

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

comment:25 Changed 10 years ago by Volker Braun

Patch needs to be rediffed for current sage...

comment:26 Changed 10 years ago by Ben Hutz

All seems good on 5.7

comment:27 Changed 10 years ago by Volker Braun

Patchbot: apply trac_13130_rings_for_projective_space.patch

comment:28 Changed 10 years ago by Ben Hutz

Minor changes from review.

Changed 10 years ago by Ben Hutz

minor changes from review

comment:29 Changed 10 years ago by Michelle Manes

Status: needs_reviewpositive_review

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.

comment:30 Changed 10 years ago by Jeroen Demeyer

Reviewers: Michelle Manes

comment:31 Changed 10 years ago by Jeroen Demeyer

Merged in: sage-5.8.beta3
Resolution: fixed
Status: positive_reviewclosed

comment:32 Changed 10 years ago by Frédéric Chapoton

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

comment:33 Changed 10 years ago by Jeroen Demeyer

I agree, the documentation formatting is horrible.

comment:34 Changed 10 years ago by Ben Hutz

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

Note: See TracTickets for help on using tickets.