Opened 2 years ago
Closed 2 years ago
#30599 closed enhancement (fixed)
Define a new data structure for a list of combinatorial faces
Reported by:  ghkliem  Owned by:  

Priority:  major  Milestone:  sage9.3 
Component:  geometry  Keywords:  face list data structure 
Cc:  Travis Scrimshaw  Merged in:  
Authors:  Jonathan Kliem  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  7a40b9a (Commits, GitHub, GitLab)  Commit:  7a40b9ae38b4f7bb76121ec294f8479fed7b8cde 
Dependencies:  #30598  Stopgaps: 
Description
We define a new data structure for a lists of combinatorial faces.
The corresponding functions will replace the functions in bit_vector_operations.cc
and now are factored out to bitsets.
See #30549 for doctesting.
Change History (7)
comment:1 Changed 2 years ago by
Cc:  Travis Scrimshaw added 

Status:  new → needs_review 
comment:2 Changed 2 years ago by
Commit:  aaa46d81bcf35da0d84c871d42fcb21bddb80322 → c81f43879c59c6da2ebc3627a5e2c3b66c3aca8b 

comment:3 followup: 5 Changed 2 years ago by
The cdef void sort_faces_list(face_list_t faces):
seems strange. I don't see why there needs to be a return
in the if
because there is no remainder of the function. Did something get lost?
Trivial, but not C :P
in cdef inline size_t get_next_level_fused
: faces.n_faces = 1;
comment:4 Changed 2 years ago by
Commit:  c81f43879c59c6da2ebc3627a5e2c3b66c3aca8b → 7a40b9ae38b4f7bb76121ec294f8479fed7b8cde 

Branch pushed to git repo; I updated commit sha1. New commits:
7a40b9a  redundant exit; removed semicolon

comment:5 Changed 2 years ago by
Replying to tscrim:
The
cdef void sort_faces_list(face_list_t faces):
seems strange. I don't see why there needs to be areturn
in theif
because there is no remainder of the function. Did something get lost?Trivial, but not C
:P
incdef inline size_t get_next_level_fused
:faces.n_faces = 1;
Done.
The return
is still needed, but only in one place (out of two). In sort_faces_list
it is indeed redundant. A few lines down it is still present.
comment:6 Changed 2 years ago by
Reviewers:  → Travis Scrimshaw 

Status:  needs_review → positive_review 
I agree that it is needed for _sort_faces_loop
.
Thank you. LGTM.
comment:7 Changed 2 years ago by
Branch:  u/ghkliem/data_structure_for_face_list → 7a40b9ae38b4f7bb76121ec294f8479fed7b8cde 

Resolution:  → fixed 
Status:  positive_review → closed 
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
put imports in order
import instead of include
new data structure for a combinatorial face of a polyhedron
put imports in order
added file
face_list_t
add find and sort
fixes to list_of_faces.pxi
remove pxi file
move not inlined functions to pyx file