Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#13130 closed enhancement (fixed)

Ring support for projective space points and morphisms

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

Description (last modified by bhutz)

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 annahaensch 5 years ago.
Replaces trac_13130_rings_for_projective_space.patch
trac_13130_rings_for_projective_space.patch (41.8 KB) - added by bhutz 5 years ago.
minor changes from review

Download all attachments as: .zip

Change History (36)

comment:1 Changed 6 years ago by 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

Last edited 5 years ago by bhutz (previous) (diff)

comment:2 Changed 6 years ago by bhutz

  • Status changed from new to needs_review

comment:3 Changed 6 years ago by novoselt

  • Authors changed from bhutz to Ben Hutz

comment:4 Changed 5 years ago by bhutz

Updated so that it applies on Sage-5.1

Changed 5 years ago by annahaensch

Replaces trac_13130_rings_for_projective_space.patch

comment:5 Changed 5 years ago by 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` .

comment:6 Changed 5 years ago by annahaensch

  • Status changed from needs_review to needs_work

comment:7 Changed 5 years ago by 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?

comment:8 Changed 5 years ago by 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.

comment:9 follow-up: Changed 5 years ago by 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?

comment:10 in reply to: ↑ 9 Changed 5 years ago by 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.

comment:11 follow-up: Changed 5 years ago by 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?

comment:12 in reply to: ↑ 11 ; follow-up: Changed 5 years ago by 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.

comment:13 in reply to: ↑ 12 Changed 5 years ago by 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?

comment:14 Changed 5 years ago by 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.

comment:15 Changed 5 years ago by 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?

comment:16 Changed 5 years ago by 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.

comment:17 Changed 5 years ago by 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?

comment:18 Changed 5 years ago by 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.

comment:19 Changed 5 years ago by bhutz

  • Description modified (diff)

comment:20 Changed 5 years ago by bhutz

  • Status changed from needs_work to needs_review

comment:21 Changed 5 years ago by minz

  • Cc minz added

comment:22 Changed 5 years ago by 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.

comment:23 Changed 5 years ago by bhutz

Updated to work with Sage 5.5.

comment:24 Changed 5 years ago by bhutz

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

comment:25 Changed 5 years ago by vbraun

Patch needs to be rediffed for current sage...

comment:26 Changed 5 years ago by bhutz

All seems good on 5.7

comment:27 Changed 5 years ago by vbraun

Patchbot: apply trac_13130_rings_for_projective_space.patch

comment:28 Changed 5 years ago by bhutz

Minor changes from review.

Changed 5 years ago by bhutz

minor changes from review

comment:29 Changed 5 years ago by mmanes

  • Status changed from needs_review to positive_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 5 years ago by jdemeyer

  • Reviewers set to Michelle Manes

comment:31 Changed 5 years ago by jdemeyer

  • Merged in set to sage-5.8.beta3
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:32 Changed 5 years ago by chapoton

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

comment:33 Changed 5 years ago by jdemeyer

I agree, the documentation formatting is horrible.

comment:34 Changed 5 years ago by bhutz

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

Note: See TracTickets for help on using tickets.