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

Merge duplications in edges, ridges and f-vector of combinatorial polyhedron #30445

Closed
kliem opened this issue Aug 26, 2020 · 6 comments
Closed

Comments

@kliem
Copy link
Contributor

kliem commented Aug 26, 2020

We merge some duplications of obtaining edges, ridges and f-vector.

There was an entire copy of getting edges/ridges that also obtained the f-vector. But it is much simpler doing this directly in the f-vector code.

Also getting the edges/ridges is almost the same thing. We can let the FaceIterator worry about the details.

Depends on #30443

CC: @jplab @LaisRast @tscrim

Component: geometry

Keywords: code duplication, combinatorial polyhedron

Author: Jonathan Kliem

Branch/Commit: 62b3d9f

Reviewer: Travis Scrimshaw

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

@kliem kliem added this to the sage-9.2 milestone Aug 26, 2020
@mkoeppe mkoeppe modified the milestones: sage-9.2, sage-9.3 Oct 24, 2020
@tscrim
Copy link
Collaborator

tscrim commented Jan 7, 2021

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Jan 7, 2021

comment:4

Some minor formatting things (which you can either change or ignore and then set this to a positive review):

-    cdef size_t _compute_edges_or_ridges_with_iterator(self, FaceIterator face_iter, bint do_atom_rep, size_t ***edges_pt, size_t *counter_pt, size_t *current_length_pt, MemoryAllocator mem) except -1:
+    cdef size_t _compute_edges_or_ridges_with_iterator(self, FaceIterator face_iter,
+                                                       bint do_atom_rep,
+                                                       size_t ***edges_pt,
+                                                       size_t *counter_pt,
+                                                       size_t *current_length_pt,
+                                                       MemoryAllocator mem) except -1:
         r"""
         See :meth:`CombinatorialPolyhedron._compute_edges`.
         """
         cdef size_t a,b                # facets of an edge
         cdef int dim = self.dimension()
         cdef output_dimension = 1 if do_atom_rep else dim - 2
 
-        while (face_iter.next_dimension() == output_dimension):
+        while face_iter.next_dimension() == output_dimension:

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 7, 2021

Branch pushed to git repo; I updated commit sha1. New commits:

cbc711dMerge branch 'u/gh-kliem/merge_edges_ridges_f_vector' of git://trac.sagemath.org/sage into u/gh-kliem/merge_edges_ridges_f_vector
62b3d9fformating

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 7, 2021

Changed commit from 8814532 to 62b3d9f

@kliem
Copy link
Contributor Author

kliem commented Jan 7, 2021

comment:6

Thank you.

@vbraun
Copy link
Member

vbraun commented Jan 17, 2021

Changed branch from u/gh-kliem/merge_edges_ridges_f_vector to 62b3d9f

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

4 participants