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

polyhedral: more standard constructions #2591

Merged
merged 70 commits into from
Aug 21, 2023
Merged

polyhedral: more standard constructions #2591

merged 70 commits into from
Aug 21, 2023

Conversation

micjoswig
Copy link
Member

@micjoswig micjoswig commented Jul 26, 2023

Many new constructions of polytopes were added.

TODO:

  • add more
  • defer long_and_winding (because of coordinate type)
  • adjust names, i.e., add "polytope" more often

deferred generalized_permutahedron (because of type conversion issue)

Project.toml Outdated Show resolved Hide resolved
docs/src/PolyhedralGeometry/Polyhedra/constructions.md Outdated Show resolved Hide resolved
docs/src/PolyhedralGeometry/Polyhedra/constructions.md Outdated Show resolved Hide resolved
test/PolyhedralGeometry/polyhedron.jl Outdated Show resolved Hide resolved
test/PolyhedralGeometry/polyhedron.jl Outdated Show resolved Hide resolved
test/PolyhedralGeometry/polyhedron.jl Outdated Show resolved Hide resolved
test/PolyhedralGeometry/polyhedron.jl Outdated Show resolved Hide resolved
test/PolyhedralGeometry/polyhedron.jl Outdated Show resolved Hide resolved
lkastner and others added 6 commits August 18, 2023 12:51
Co-authored-by: Benjamin Lorenz <benlorenz@users.noreply.github.com>
Co-authored-by: Lars Göttgens <lars.goettgens@rwth-aachen.de>
Remove multiplex_polytope and neighborly_cubical_polytope since these
come without coordinates and lack many other properties as well. Hence
they are almost unusable in Oscar.
Sanitize the input dimension for associahedron as "the polymake error
message is not helpful"
- Remove unseeded tests
- Add seeds
- sort output
@lkastner lkastner marked this pull request as ready for review August 19, 2023 15:21
@lgoettgens
Copy link
Member

I noticed that parts of the additions are inconsistently formatted. @lkastner could you maybe run the formatter on the large chunks of additions?

@lkastner
Copy link
Member

I noticed that parts of the additions are inconsistently formatted. @lkastner could you maybe run the formatter on the large chunks of additions?

Again, this is an Oscar wide issue, so why remark it here if the rest of the code is not up to this standard? Furthermore this issue could be solved by having appropriate checks placed in the testsuite. Additionally we have nothing in the docs on JuliaFormatter, which I assume you meant, so I added a paragraph in the styleguide. Out of curiosity I tried to run JuliaFormatter on the entire codebase and it even throws an error.

I do not think this level of feedback is sustainable. Do you want to remark on these formatting / spacing / whitespace issues on every (many) PR(s) in the future?

@lgoettgens
Copy link
Member

I noticed that parts of the additions are inconsistently formatted. @lkastner could you maybe run the formatter on the large chunks of additions?

Again, this is an Oscar wide issue, so why remark it here if the rest of the code is not up to this standard? Furthermore this issue could be solved by having appropriate checks placed in the testsuite. Additionally we have nothing in the docs on JuliaFormatter, which I assume you meant, so I added a paragraph in the styleguide. Out of curiosity I tried to run JuliaFormatter on the entire codebase and it even throws an error.

I do not think this level of feedback is sustainable. Do you want to remark on these formatting / spacing / whitespace issues on every (many) PR(s) in the future?

If I remember correctly, I discussed that with some people a while ago (cannot remember who) and the conclusion was more or less: We do not run the formatter on everything at some point, as that creates conflicts with all open PRs and makes git blame basically unusable. Instead, new additions should follow the formatting styleguide, and whenever you touch large parts of a function you may format that as well.

However, I do not think this is the right place to discuss it. We should move that to a discussion thread, slack, or a Friday meeting (unfortunately I cannot participate this and next week due to travel), to get a broader range of opinions and then write the result down somewhere, e.g. the dev docs.

@lkastner lkastner merged commit d71310a into master Aug 21, 2023
9 of 12 checks passed
@lkastner lkastner deleted the zkg/associahedron branch August 21, 2023 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request topic: polyhedral geometry Issue concerns polyhedral geometry code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants