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.

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

No more maximal dimension requirement for lattice polytopes #6831

Closed
novoselt opened this issue Aug 27, 2009 · 10 comments
Closed

No more maximal dimension requirement for lattice polytopes #6831

novoselt opened this issue Aug 27, 2009 · 10 comments

Comments

@novoselt
Copy link
Member

Since PALP requires polytopes to have the same dimension as the ambient space, LatticePolytope class required it as well. This patch drops this requirement by storing an internal copy of the same polytope in some sublattice basis and using it when necessary to call PALP. Quite a few functions had to be updated, I tried to add new doctests to check most of the new branches of code.

This patch will be a prerequisite for some code for working with nef partitions which I hope to submit in the future.

Component: geometry

Author: Andrey Novoseltsev

Reviewer: Marshall Hampton

Merged: sage-4.2.1.alpha0

Issue created by migration from https://trac.sagemath.org/ticket/6831

@sagetrac-mhampton
Copy link
Mannequin

sagetrac-mhampton mannequin commented Oct 29, 2009

comment:1

Attachment: trac_6831_allow_non_full_dimensional_lattice_polytopes.patch.gz

Can you rebase this against sage-4.2? I reviewed some of your patches previously, and some made them in, which means this doesn't apply against the current version. Sorry about that, I'm hoping to finish up reviewing your patches for 4.2.1.

-Marshall

@novoselt
Copy link
Member Author

comment:2

Actually, I have written this patch after all the previous ones were applied, I am not even sure if it will work without them.

I'll check and rebase if it does not work for 4.2, but it will take me some time.

Thanks for all the other reviews!
Andrey

@sagetrac-mhampton
Copy link
Mannequin

sagetrac-mhampton mannequin commented Oct 30, 2009

comment:3

My mercurial-foo is not that strong, so perhaps I made a mistake trying to apply the patch. But I don't understand it if so. It would be OK to have a rebased patch that included other changes - even if they aren't positively reviewed yet I could do them all at once, assuming they mainly affected lattice_polytope.py.

-Marshall

@novoselt
Copy link
Member Author

Rebased patch, also includes patches for tickets 6779, 6780, and 6805

@novoselt
Copy link
Member Author

comment:4

Attachment: trac_6831_allow_non_full_dimensional_lattice_polytopes-4.2-rebase.patch.gz

This patch should work on 4.2 without any prior requirements, it includes changes made in 3 other small tickets which you have already given a positive review - I will put comments in those tickets. Let me know if I should do anything else.

Andrey

@sagetrac-mhampton
Copy link
Mannequin

sagetrac-mhampton mannequin commented Oct 30, 2009

comment:5

Great, thanks. I can't do it today but I'll try to review it this weekend.

-Marshall

@sagetrac-mhampton
Copy link
Mannequin

sagetrac-mhampton mannequin commented Nov 1, 2009

comment:6

OK, this looks good, positive review. I've looked it over, did some manual tests, it passes its own doctests and has 100% coverage. I rebuilt the reference manual and it looks good there. Seems like all the changes are for the better.

@mwhansen
Copy link
Contributor

mwhansen commented Nov 2, 2009

Reviewer: Marshall Hampton

@mwhansen
Copy link
Contributor

mwhansen commented Nov 2, 2009

Merged: sage-4.2.1.alpha0

@mwhansen
Copy link
Contributor

mwhansen commented Nov 2, 2009

Author: Andrey Novoseltsev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants