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:

Status badges

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)

trac_6831_allow_non_full_dimensional_lattice_polytopes.patch (38.5 KB) - added by novoselt 12 years ago.
trac_6831_allow_non_full_dimensional_lattice_polytopes-4.2-rebase.patch (43.5 KB) - added by novoselt 12 years ago.
Rebased patch, also includes patches for tickets 6779, 6780, and 6805

Download all attachments as: .zip

Change History (9)

comment:1 Changed 12 years ago by mhampton

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

comment:2 Changed 12 years ago by novoselt

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 mhampton

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

Changed 12 years ago by novoselt

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

comment:4 Changed 12 years ago by novoselt

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 mhampton

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 mhampton

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

  • Authors set to Andrey Novoseltsev
  • 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
Note: See TracTickets for help on using tickets.