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: sage-6.1
Component: algebraic geometry Keywords: sage-days55, 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 bhutz)

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)

trac_15396_an_element_for_aff_proj_spaces.patch (2.2 KB) - added by bhutz 5 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 5 years ago by bhutz

  • Description modified (diff)

comment:2 Changed 5 years ago by bhutz

  • Authors set to bhutz
  • Status changed from new to needs_review

comment:3 Changed 5 years ago by chapoton

  • Branch set to u/chapoton/15396
  • Commit set to 82b1a730e73cc6f57b64e6ed6fb6639a1103f09c

I have made a git branch, and then a review commit on top of it


New commits:

8a0466dTrac #15396: implement an_element for affine and projective spaces
82b1a73trac #15396 first review patch

comment:4 Changed 5 years ago by bhutz

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 chapoton

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 bhutz

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 chapoton

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 bhutz

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,n-1,n-2,...,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 chapoton

mmm. As far as I know, GL_N(F_2) is not a ring..

comment:10 Changed 5 years ago by bhutz

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 git

  • Commit changed from 82b1a730e73cc6f57b64e6ed6fb6639a1103f09c to dd22558ffeadbe916b50b3d1338816765e0a5ce7

Branch pushed to git repo; I updated commit sha1. New commits:

dd22558trac #15396 proposal for more typical elements

comment:12 Changed 5 years ago by chapoton

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 chapoton

  • 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 chapoton

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 bhutz

  • Authors changed from bhutz to Ben Hutz

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 bhutz

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 chapoton

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 bhutz

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 chapoton

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 bhutz

  • Status changed from needs_review to positive_review

If they all appear, then that is fine.

comment:21 Changed 5 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.