Opened 12 years ago
Closed 12 years ago
#6831 closed enhancement (fixed)
No more maximal dimension requirement for lattice polytopes
Reported by: | novoselt | Owned by: | mhampton |
---|---|---|---|
Priority: | major | Milestone: | sage-4.2.1 |
Component: | geometry | Keywords: | |
Cc: | Merged in: | sage-4.2.1.alpha0 | |
Authors: | Andrey Novoseltsev | Reviewers: | Marshall Hampton |
Report Upstream: | Work issues: | ||
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
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.
Attachments (2)
Change History (9)
Changed 12 years ago by
comment:1 Changed 12 years ago by
comment:2 Changed 12 years ago by
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
comment:3 Changed 12 years ago by
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
comment:4 Changed 12 years ago by
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
comment:5 Changed 12 years ago by
Great, thanks. I can't do it today but I'll try to review it this weekend.
-Marshall
comment:6 Changed 12 years ago by
- Status changed from needs_review to positive_review
- Summary changed from [with patch, needs review] No more maximal dimension requirement for lattice polytopes to No more maximal dimension requirement for lattice polytopes
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.
comment:7 Changed 12 years ago by
- Merged in set to sage-4.2.1.alpha0
- Resolution set to fixed
- Reviewers set to Marshall Hampton
- Status changed from positive_review to closed
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