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

Light Cell Geometry #1499

Merged
merged 36 commits into from
Jul 1, 2020

Conversation

JacquesOlivierLachaud
Copy link
Member

@JacquesOlivierLachaud JacquesOlivierLachaud commented Jun 5, 2020

PR Description

This PR changes the data structure for storing cell covers in CellGeometry. Previous one was a CubicalComplex, which is based on a std::unordered_map Cell -> Data (here Data is not used). We use instead a new class UnorderedSetByBlock to store set of Points (Cells are represented by their Khalimsky points). The UnorderedSetByBlock is an std::unordered_map Point -> Word, where points are gathered by blocks of 32 consecutive points along x axis. If any point in a block is present, the corresponding element is present and bits to 1 in the word correspond to points present in the set. On average, memory occupancy is divided by four, the structure is four times faster for traversal and approximately same speed for queries/insertion/erase.
CellGeometry and DigitalConvexity use now this data structure. A drawback is that this version is not fully backward compatible with the first version since the few operations involving CubicalComplex are removed.

Checklist

  • Unit-test of your feature with Catch.
  • Doxygen documentation of the code completed (classes, methods, types, members...)
  • Documentation module page added or updated.
  • New entry in the ChangeLog.md added.
  • No warning raised in Debug cmake mode (otherwise, Travis C.I. will fail).
  • All continuous integration tests pass (Travis & appveyor)

@JacquesOlivierLachaud JacquesOlivierLachaud changed the title Light Cell Geometry [WIP] Light Cell Geometry Jun 5, 2020
@JacquesOlivierLachaud JacquesOlivierLachaud changed the title [WIP] Light Cell Geometry Light Cell Geometry Jun 6, 2020
@dcoeurjo dcoeurjo self-requested a review June 10, 2020 15:38
Copy link
Member

@dcoeurjo dcoeurjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice PR. Couple of comments..

src/DGtal/geometry/volumes/CellGeometry.h Outdated Show resolved Hide resolved
src/DGtal/geometry/volumes/CellGeometry.ih Outdated Show resolved Hide resolved
src/DGtal/geometry/volumes/CellGeometry.ih Outdated Show resolved Hide resolved
src/DGtal/geometry/volumes/DigitalConvexity.ih Outdated Show resolved Hide resolved
src/DGtal/geometry/volumes/DigitalConvexity.ih Outdated Show resolved Hide resolved
src/DGtal/geometry/volumes/DigitalConvexity.ih Outdated Show resolved Hide resolved
src/DGtal/kernel/UnorderedSetByBlock.h Outdated Show resolved Hide resolved
src/DGtal/kernel/UnorderedSetByBlock.h Outdated Show resolved Hide resolved
src/DGtal/kernel/UnorderedSetByBlock.h Outdated Show resolved Hide resolved
src/DGtal/kernel/UnorderedSetByBlock.h Outdated Show resolved Hide resolved
Copy link
Member

@rolanddenis rolanddenis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Besides the indentation issues, I have some questions about bitwise operations and an unknown random function 😉

src/DGtal/geometry/volumes/CellGeometry.h Outdated Show resolved Hide resolved
src/DGtal/geometry/volumes/CellGeometry.h Outdated Show resolved Hide resolved
src/DGtal/geometry/volumes/CellGeometry.h Outdated Show resolved Hide resolved
src/DGtal/geometry/volumes/CellGeometry.h Outdated Show resolved Hide resolved
src/DGtal/geometry/volumes/CellGeometry.h Outdated Show resolved Hide resolved
src/DGtal/kernel/UnorderedSetByBlock.h Outdated Show resolved Hide resolved
src/DGtal/kernel/UnorderedSetByBlock.h Outdated Show resolved Hide resolved
src/DGtal/kernel/UnorderedSetByBlock.h Outdated Show resolved Hide resolved
src/DGtal/kernel/UnorderedSetByBlock.h Outdated Show resolved Hide resolved
tests/kernel/testUnorderedSetByBlock.cpp Outdated Show resolved Hide resolved
Copy link
Member

@rolanddenis rolanddenis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update, some minor requested changes 👌

tests/kernel/testUnorderedSetByBlock.cpp Outdated Show resolved Hide resolved
src/DGtal/kernel/UnorderedSetByBlock.h Outdated Show resolved Hide resolved
src/DGtal/kernel/UnorderedSetByBlock.h Show resolved Hide resolved
src/DGtal/kernel/UnorderedSetByBlock.h Outdated Show resolved Hide resolved
rolanddenis
rolanddenis previously approved these changes Jun 16, 2020
@JacquesOlivierLachaud
Copy link
Member Author

Finally it works. I do not know where was the problem (since it was working fine on my laptop), so I static_cast everywhere possible. There was an implicit conversion that went bad on some system certainly. So I think the PR is ready now.

@dcoeurjo
Copy link
Member

dcoeurjo commented Jul 1, 2020

thx merging

@dcoeurjo dcoeurjo merged commit dc0d213 into DGtal-team:master Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants