Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Switch cones to PointCollection #12544

Closed
novoselt opened this issue Feb 20, 2012 · 54 comments
Closed

Switch cones to PointCollection #12544

novoselt opened this issue Feb 20, 2012 · 54 comments
Assignees
@sagetrac-mhampton
Labels
c: geometry p: major / 3 t: enhancement
Milestone
sage-5.2

Comments

@novoselt
Copy link
Member

The class PointCollection at #11400 is intended for uniform handling of ray/line generators and their subsets for cones, fans, and, eventually, lattice polytopes. This ticket is about making the switch for cones.

Apply: attachment: trac_12544_switch_cones_to_PointCollection_folded.patch

Depends on #12965

CC: @vbraun

Component: geometry

Keywords: toric, sd40.5

Author: Andrey Novoseltsev

Reviewer: Volker Braun

Merged: sage-5.2.beta1

Issue created by migration from https://trac.sagemath.org/ticket/12544
@novoselt novoselt added this to the sage-5.1 milestone Feb 20, 2012
@novoselt
Copy link
Member Author

comment:1

So far the patch is "proof of concept" and with the alternative solution on #12541 it passes all the tests for cones and fans (except for like 60 fails due to changed output formatting). This is exactly what was intended: most things should work as if cone.rays() was still a tuple, but now we can tweak representation and latexing, add extra methods, and suffer no performance hit as it would be with Sequence.

@novoselt
Copy link
Member Author

Dependencies: #12541

@novoselt

This comment has been minimized.

Sign in to view
@novoselt
Copy link
Member Author

novoselt commented Mar 7, 2012

Changed dependencies from #12541 to #11599, #12541

@novoselt
Copy link
Member Author

novoselt commented Mar 7, 2012

comment:3

I've tried to break changes into several patches, although they got bigger and bigger. However, despite of the total size, these patches should be easy to review as they don't do anything deep - a few conversion methods, a few deprecations, and a lot of doctest changes due to different output formatting.

Personally, I am quite pleased with the results - while some calls got a few more characters, one of the most important methods for cones and fans now produces convenient output with a detailed but not repeated description of the ambient lattice. This worked especially well for quotients and sublattices, whose description was not obvious from ray output before. Hopefully, that's not just my point of view and the changes are objectively for the best ;-)

I have #11599 in the beginning of my queue now, so this ticket is based on top of it.

@novoselt
Copy link
Member Author

Changed dependencies from #11599, #12541 to #11599, #11634, #12541

@novoselt
Copy link
Member Author

comment:4

Rebased to apply on top of the positively reviewed #11634

@novoselt
Copy link
Member Author

comment:5

... and removed added trailing whitespaces.

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Mar 13, 2012

comment:6
applying trac_12544_deprecate_old_ray_methods.patch
patching file sage/geometry/cone.py
Hunk #42 FAILED at 3787
1 out of 44 hunks FAILED -- saving rejects to file sage/geometry/cone.py.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh trac_12544_deprecate_old_ray_methods.patch

(My guess is that you removed a trailing whitespace from a line in an earlier patch that was used as a context line by this later one.)

@loefflerd loefflerd mannequin added s: needs work and removed s: needs review labels Mar 13, 2012
@novoselt
Copy link
Member Author

comment:7

Grrr... That's precisely what has happened. New versions are made using hg export and seem to apply fine...

@novoselt
Copy link
Member Author

comment:8

Rebased on top of #12361, which is almost done.

@novoselt
Copy link
Member Author

Changed dependencies from #11599, #11634, #12541 to #11599, #11634, #12541, #12361

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Mar 26, 2012

comment:9

According to the patchbot this is failing to apply to 5.0.beta10 -- any idea what that's about?

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Mar 26, 2012

Work Issues: patch does not apply

@loefflerd loefflerd mannequin added s: needs work and removed s: needs review labels Mar 26, 2012
@novoselt
Copy link
Member Author

comment:10

I don't know - maybe I have saved some changes in the previous patches as well. I've reexported versions that apply to beta10 for me without any problems.

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Mar 26, 2012

Changed work issues from patch does not apply to none

@loefflerd loefflerd mannequin added s: needs review and removed s: needs work labels Mar 26, 2012
@novoselt
Copy link
Member Author

Attachment: trac_12544_switch_cones_to_PointCollection_folded.patch.gz

@novoselt
Copy link
Member Author

comment:30

OK, ready to go!

@novoselt
Copy link
Member Author

Changed work issues from output formatting, base rings to none

@novoselt
Copy link
Member Author

comment:31

Patchbot, apply only trac_12544_switch_cones_to_PointCollection_folded.patch

@novoselt
Copy link
Member Author

comment:32

Apply trac_12544_switch_cones_to_PointCollection_folded.patch

@vbraun
Copy link
Member

vbraun commented Jun 29, 2012

comment:33

Apply trac_12544_switch_cones_to_PointCollection_folded.patch

@vbraun

This comment has been minimized.

Sign in to view
@novoselt
Copy link
Member Author

comment:35

The patchbot positively hates this ticket and does not want to understand that only last patch has to be applied...

@vbraun
Copy link
Member

vbraun commented Jun 29, 2012

Reviewer: Volker Braun

@vbraun
Copy link
Member

vbraun commented Jun 29, 2012

comment:36

Looks good to me.

@vbraun vbraun modified the milestones: sage-5.1, sage-5.2 Jun 29, 2012
@jdemeyer
Copy link

jdemeyer commented Jul 1, 2012

Changed dependencies from #11599, #11634, #12541, #12361, #12965 to #12965

@robertwb
Copy link
Contributor

robertwb commented Jul 3, 2012

comment:38

Apply only trac_12544_switch_cones_to_PointCollection_folded.patch

@robertwb
Copy link
Contributor

robertwb commented Jul 3, 2012

comment:39

apply trac_12544_switch_cones_to_PointCollection_folded.patch

@jdemeyer

This comment has been minimized.

Sign in to view
@novoselt
Copy link
Member Author

novoselt commented Jul 3, 2012

comment:41

So - any clue what is wrong with the patchbot here?

@jdemeyer
Copy link

jdemeyer commented Jul 7, 2012

Merged: sage-5.2.beta1

@jdemeyer jdemeyer closed this as completed Jul 7, 2012
@vbraun vbraun mentioned this issue May 8, 2013
Closed
@sagetrac-eisermbi sagetrac-eisermbi mannequin mentioned this issue Jul 16, 2012
Closed
@vbraun vbraun mentioned this issue Jul 16, 2012
Closed
@novoselt novoselt mentioned this issue Mar 26, 2013
Closed
This was referenced Aug 14, 2012
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sagetrac-mhampton sagetrac-mhampton

Labels
c: geometry p: major / 3 t: enhancement
Projects
None yet
Milestone
sage-5.2
Development

No branches or pull requests
4 participants
@robertwb @vbraun @novoselt @jdemeyer