-
-
Notifications
You must be signed in to change notification settings - Fork 480
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
Remove basic access to bitsets in combinatorial polyhedron #30528
Comments
Reviewer: Travis Scrimshaw |
comment:2
This LGTM although it is making me wonder if we should have our own bitset operations class in Sage. There is the C++ STL bitset, and if we do need something slightly more custom, it would be good as a C++ library that we would then interface with. |
comment:3
Thank you.
The idea of this ticket (and a follow up) is to get to the point, where it makes sense to try different things. Currently, whenever I try something different, there is so many places that need to be adjusted. This is annoying. So the idea is that For a good face iterator I need the following:
|
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. Last 10 new commits:
|
comment:5
Rebased. |
comment:6
Thank you for the explanation and the rebase. |
comment:7
Replying to @kliem:
A quick update to the explanation: I tested roaring and it performs wonderful, but only with bitsets starting with about 50,000 bits, as it stores in chunks and the chunks are set to 2^16 bits each. So this is not an option for small stuff, but very interesting if you go into very large things. |
comment:8
Doesn't compile on osx
|
comment:10
Ok, falling back to a manual It really doesn't matter that much, because it is not less efficient then the current one and it will hopefully be removed in follow up. This ticket is mostly about moving some function out of the "frontend", to account for the fact that |
comment:11
@mkoeppe: Could you maybe check if this works on OSX? So if it compiles now and if tests in |
comment:13
https://github.com/kliem/sage/actions/runs/374613473 Eventually this will also terminate. |
comment:14
Ok. Seems to work now. The OSX patchbot came back and it is morally green (unrelated tests fail). |
comment:15
Fails on 32-bit:
|
comment:17
I honestly don't want to keep working on this anymore. This was supposed to make #30549 a bit easier, but apparently this is not happening.
|
Changed author from Jonathan Kliem to none |
Changed commit from |
Changed branch from u/gh-kliem/no_more_basic_access to none |
comment:18
As you wish. |
This ticket removes basic access to bitsets in files related to combinatorial polyhedron and moves this to a new file
combinatorial_polyhedron/bitsets.cc
, which is more or less a copy of parts ofdata_structures/bitsets.pxi
.Also
bit_vector_operations.cc
is renamed tobitset_operations.cc
.bitsets.cc
should be a C++ file, becausebitset_operations.cc
will use it (and the later one needs to be C++ or C, because it needs to be able to handle macros as indicated in #27103).Follow ups:
get_next_level
.Currently this is
(uin64_t**, size_t, uint64_t**, uint64_t**, size_t, size_t)
and much worse for the simple/simplicial case (Improve face iterator for simple/simplicial polytopes #30040). Eventually this should be similar to(face_list_struct, face_list_struct, face_list_struct)
for both cases and then the underlying function can worry aboutface_list_struct
. This is also more solid with regard to future changes.bitsets
and only having small parts in an extra C++ file that can be optimized by intrinsics.CC: @tscrim @mkoeppe
Component: geometry
Reviewer: Travis Scrimshaw
Issue created by migration from https://trac.sagemath.org/ticket/30528
The text was updated successfully, but these errors were encountered: