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

Define a new data structure for a list of combinatorial faces #30599

Closed
kliem opened this issue Sep 18, 2020 · 9 comments
Closed

Define a new data structure for a list of combinatorial faces #30599

kliem opened this issue Sep 18, 2020 · 9 comments

Comments

@kliem
Copy link
Contributor

kliem commented Sep 18, 2020

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.

Depends on #30598

CC: @tscrim

Component: geometry

Keywords: face list data structure

Author: Jonathan Kliem

Branch/Commit: 7a40b9a

Reviewer: Travis Scrimshaw

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

@kliem kliem added this to the sage-9.3 milestone Sep 18, 2020
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 18, 2020

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 18, 2020

Changed commit from aaa46d8 to c81f438

@tscrim
Copy link
Collaborator

tscrim commented Oct 24, 2020

comment:3

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;

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 26, 2020

Changed commit from c81f438 to 7a40b9a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 26, 2020

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

7a40b9aredundant exit; removed semicolon

@kliem
Copy link
Contributor Author

kliem commented Oct 26, 2020

comment:5

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.

@tscrim
Copy link
Collaborator

tscrim commented Oct 27, 2020

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Oct 27, 2020

comment:6

I agree that it is needed for _sort_faces_loop.

Thank you. LGTM.

@vbraun
Copy link
Member

vbraun commented Nov 1, 2020

Changed branch from u/gh-kliem/data_structure_for_face_list to 7a40b9a

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

3 participants