Opened 5 years ago
Closed 5 years ago
#15396 closed defect (fixed)
fix .an_element for affine and projective spaces
Reported by:  bhutz  Owned by:  bhutz 

Priority:  minor  Milestone:  sage6.1 
Component:  algebraic geometry  Keywords:  sagedays55, TestSuite 
Cc:  Merged in:  
Authors:  Ben Hutz  Reviewers:  Frédéric Chapoton 
Report Upstream:  N/A  Work issues:  
Branch:  u/chapoton/15396  Commit:  dd22558ffeadbe916b50b3d1338816765e0a5ce7 
Dependencies:  Stopgaps: 
Description (last modified by )
There are two failures when the TestSuite? is run on affine or projective space. They are test_an_element
, test_some_elements
. The first just needs an implementation of an_element
which is done in this ticket. The second seems to be an underlying issue with test_category
and will not be addressed here.
Attachments (1)
Change History (22)
Changed 5 years ago by
comment:1 Changed 5 years ago by
 Description modified (diff)
comment:2 Changed 5 years ago by
 Status changed from new to needs_review
comment:3 Changed 5 years ago by
 Branch set to u/chapoton/15396
 Commit set to 82b1a730e73cc6f57b64e6ed6fb6639a1103f09c
comment:4 Changed 5 years ago by
So, what now? Do I review the review changes and mark as positive if I think they are fine?
btw, the reviewer field is still blank.
comment:5 Changed 5 years ago by
Well, I am not quite totally happy with the given elements. Maybe one can manage to make them more generic in some way ? Something like
self([(5  i)*R.an_element() for i in range(n)])
maybe ?
comment:6 Changed 5 years ago by
I understand what you are saying, but it is unclear to me what that would take. Since affine and projective space can be defined over essentially any ring R
, multiplying R.an_element()
by an integer may or may not be appropriate. Using R.random_element()
also didn't seem appropriate for the an.element()
function, but is likely a better choice than multiplying by integers without knowing what the base ring is.
comment:7 Changed 5 years ago by
Well, if you multiply by always the same integer, this can of course give always zero.
My proposal ensures that at least one half of the coordinates is not zero, because the difference between consecutive terms is just R.an_element().
comment:8 Changed 5 years ago by
Yes, in just about any instance I can think of it will be fine. But, what if the base ring is something like GL_n(F_2)
so that multiplying by 2 gives you the zero matrix which is not in the ring, i.e. 2*R.an_element()
does not exist.
I see now you are also removing the last coordinateR.one()
from my code, in this case all you are really defining in the point (n,n1,n2,...,1,0)
since in projective space you can just factor out the R.an_element()
from each coordinate. So you certainly cannot just have some multiple of R.an_element()
for every coordinate.
comment:9 Changed 5 years ago by
mmm. As far as I know, GL_N(F_2) is not a ring..
comment:10 Changed 5 years ago by
yes, so that's a bad example. I was trying to think of something easy where multiplying by an integer was bad.
comment:11 Changed 5 years ago by
 Commit changed from 82b1a730e73cc6f57b64e6ed6fb6639a1103f09c to dd22558ffeadbe916b50b3d1338816765e0a5ce7
Branch pushed to git repo; I updated commit sha1. New commits:
dd22558  trac #15396 proposal for more typical elements

comment:12 Changed 5 years ago by
ok, here is my proposal. If you are happy with that, you can set a positive review (please check the TestSuite? first).
comment:13 Changed 5 years ago by
 Reviewers set to Frédéric Chapoton
and do not forget to fill you real name as author.
comment:14 Changed 5 years ago by
This does not seem to solve the TestSuite? problem (neither for your original methods nor for my modified methods).
comment:15 Changed 5 years ago by
I will run the the tests soon, but my original fix had fixed the problem for me. Just to double check, there are two issues with the testsuite (see original description above), this is only fixing the issue with test_an_element()
and not test_some_elements()
.
comment:16 Changed 5 years ago by
I was able to run this tonight. I'm getting the test_an_element()
is fixed and the test_category()
is still causing the other failure. The other failure is not addressed here.
Is this what you got?
comment:17 Changed 5 years ago by
Yes, I got the same. So this ticket should be good to go. The remaining issue will be considered in another ticket.
comment:18 Changed 5 years ago by
I can't think of a ring where this fails to work, so we'll go with your proposal.
Sorry for one last question, but I'm still getting used to the git reviews. Is the commit message associated with this the one on the final code change, i.e. "trac 15396 proposal for more typical elements"? If so, I'll update it before marking positive.
comment:19 Changed 5 years ago by
I do not know. Maybe this is not so important anyway. All commits will appear in the history. If you want something very clean, it is possible (but not recommended, I think) to smash all 3 commits into one using
git rebase i develop
and replacing two "apply" with "s" (standing for "stash") in the file that will open.
Once again, I think this is not necessary, and things are good as they stand now.
comment:20 Changed 5 years ago by
 Status changed from needs_review to positive_review
If they all appear, then that is fine.
comment:21 Changed 5 years ago by
 Resolution set to fixed
 Status changed from positive_review to closed
I have made a git branch, and then a review commit on top of it
New commits:
Trac #15396: implement an_element for affine and projective spaces
trac #15396 first review patch