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: gh-kliem Owned by:
Priority: major Milestone: sage-9.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:

Status badges

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 gh-kliem

Cc: Travis Scrimshaw added
Status: newneeds_review

comment:2 Changed 2 years ago by git

Commit: aaa46d81bcf35da0d84c871d42fcb21bddb80322c81f43879c59c6da2ebc3627a5e2c3b66c3aca8b

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

a369269put imports in order
bb33866import instead of include
689bd4dnew data structure for a combinatorial face of a polyhedron
d644669put imports in order
7cc7fcdadded file
f1f9499face_list_t
02eabffadd find and sort
50317b0fixes to list_of_faces.pxi
32cf17eremove pxi file
c81f438move not inlined functions to pyx file

comment:3 Changed 2 years ago by Travis Scrimshaw

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 git

Commit: c81f43879c59c6da2ebc3627a5e2c3b66c3aca8b7a40b9ae38b4f7bb76121ec294f8479fed7b8cde

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

7a40b9aredundant exit; removed semicolon

comment:5 in reply to:  3 Changed 2 years ago by gh-kliem

Replying to tscrim:

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;

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 Travis Scrimshaw

Reviewers: Travis Scrimshaw
Status: needs_reviewpositive_review

I agree that it is needed for _sort_faces_loop.

Thank you. LGTM.

comment:7 Changed 2 years ago by Volker Braun

Branch: u/gh-kliem/data_structure_for_face_list7a40b9ae38b4f7bb76121ec294f8479fed7b8cde
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.