Opened 11 months ago

Closed 9 months ago

#30458 closed enhancement (fixed)

Outsource some functions in bit_vector_operations.cc

Reported by: gh-kliem Owned by:
Priority: minor Milestone: sage-9.3
Component: geometry Keywords: code duplication, combinatorial polyhedron
Cc: Merged in:
Authors: Jonathan Kliem Reviewers: Travis Scrimshaw, Samuel Lelièvre
Report Upstream: N/A Work issues:
Branch: 5311cdf (Commits, GitHub, GitLab) Commit: 5311cdf89f1ae38c4d8b8a458b2071fa4f9f44b9
Dependencies: Stopgaps:

Status badges

Description

We simplify obtaining the inclusion maximal faces in the function get_next_level by outsourcing to new function is_contained_in_one.

In addition we implement contains_one for #30040.

This ticket merges cleanly with #30435.

Change History (13)

comment:1 Changed 11 months ago by gh-kliem

  • Branch set to u/gh-kliem/outsource_inclusion_maximal
  • Commit set to 20a39b679f660e6b59ba7db4f7c302ab454fe28b
  • Status changed from new to needs_review

New commits:

20a39b6outsource inclusion maximal

comment:2 Changed 11 months ago by git

  • Commit changed from 20a39b679f660e6b59ba7db4f7c302ab454fe28b to b672fcaf65e2219644166a6ed5d8e29c13b4d67d

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

b672fcaremoved redundant function

comment:3 Changed 11 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM.

comment:4 Changed 11 months ago by gh-kliem

Thank you.

comment:5 Changed 11 months ago by slelievre

  • Status changed from positive_review to needs_work

The functions inline int is_contained_in_one (with or without skip) have this fine description:

     Return whether ``face`` is contained in one of ``faces``.

Descriptions for the functions contains_one (with or without skip) have a problem:

 inline int contains_one(uint64_t *face, uint64_t **faces, size_t n_faces, size_t face_length){
     /*
-    Return whether ``face`` contains in one of ``faces``.
+    Return whether ``face`` contains one of ``faces``.
     */
 inline int contains_one(uint64_t *face, uint64_t **faces, size_t n_faces, size_t face_length, size_t skip){
     /*
-    Return whether ``face`` is contains in one of ``faces``.
+    Return whether ``face`` contains one of ``faces``.
 
     Skips ``faces[skip]``.
     */

comment:6 Changed 11 months ago by git

  • Commit changed from b672fcaf65e2219644166a6ed5d8e29c13b4d67d to b2ce23e9fafbb530e5c3deaaefb8205cc9b66852

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

b2ce23egrammar

comment:7 Changed 11 months ago by gh-kliem

  • Status changed from needs_work to needs_review

comment:8 Changed 11 months ago by slelievre

  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Samuel Lelièvre
  • Status changed from needs_review to positive_review

comment:9 Changed 11 months ago by gh-kliem

Thanks again.

comment:10 Changed 11 months ago by git

  • Commit changed from b2ce23e9fafbb530e5c3deaaefb8205cc9b66852 to 5311cdf89f1ae38c4d8b8a458b2071fa4f9f44b9
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. New commits:

7c970e3outsource inclusion maximal
92fb29cremoved redundant function
5311cdfgrammar

comment:11 Changed 11 months ago by gh-kliem

  • Status changed from needs_review to positive_review

Rebased.

comment:12 Changed 9 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:13 Changed 9 months ago by vbraun

  • Branch changed from u/gh-kliem/outsource_inclusion_maximal to 5311cdf89f1ae38c4d8b8a458b2071fa4f9f44b9
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.