#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 )
Projective points and morphisms are currently only support over fields. Support for rings is needed.
Apply:
Attachments (2)
Change History (36)
comment:1 Changed 6 years ago by
comment:2 Changed 6 years ago by
- Status changed from new to needs_review
comment:3 Changed 6 years ago by
comment:4 Changed 6 years ago by
Updated so that it applies on Sage-5.1
comment:5 Changed 6 years ago by
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 6 years ago by
- Status changed from needs_review to needs_work
comment:7 Changed 6 years ago by
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 6 years ago by
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: ↓ 10 Changed 6 years ago by
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 6 years ago by
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: ↓ 12 Changed 6 years ago by
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: ↓ 13 Changed 6 years ago by
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 6 years ago by
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 6 years ago by
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 returnrelative_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 6 years ago by
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 6 years ago by
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 6 years ago by
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 6 years ago by
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 6 years ago by
- Description modified (diff)
comment:20 Changed 6 years ago by
- Status changed from needs_work to needs_review
comment:21 Changed 6 years ago by
- Cc minz added
comment:22 Changed 6 years ago by
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 6 years ago by
Updated to work with Sage 5.5.
comment:24 Changed 6 years ago by
While working on something else, I found a bug in the patch. It's been fixed and the new patch uploaded.
comment:25 Changed 6 years ago by
Patch needs to be rediffed for current sage...
comment:26 Changed 6 years ago by
All seems good on 5.7
comment:27 Changed 6 years ago by
Patchbot: apply trac_13130_rings_for_projective_space.patch
comment:28 Changed 6 years ago by
Minor changes from review.
comment:29 Changed 6 years ago by
- 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 6 years ago by
- Reviewers set to Michelle Manes
comment:31 Changed 6 years ago by
- Merged in set to sage-5.8.beta3
- Resolution set to fixed
- Status changed from positive_review to closed
comment:32 Changed 6 years ago by
Well, this is closed, but you still need to correct the doc : there are wrong "INPUT::" everywhere.
comment:33 Changed 6 years ago by
I agree, the documentation formatting is horrible.
comment:34 Changed 6 years ago by
Ok. This will be fixed as part of #14217.
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