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

DigitalSetByAssociativeContainer #1023

Merged
merged 34 commits into from
Oct 1, 2015

Conversation

dcoeurjo
Copy link
Member

@dcoeurjo dcoeurjo commented Jul 6, 2015

Introduce a new model of digital sets using any associative container of the STL.
Basically, std::unordered_set (c++11) or boost::unordered_set (hash function based containers) can be used to store points of a digital set. Hence, just using this model of digital set speeds up the processing up to 40% (see benchmark results).

This PR also specializes std::hash and boost::hash with hash functions on DGtal points/vectors.

Once accepted, we could also remove DigitalSetBySTLSet and derive from DigitalSetByAssociativeContainer on specialized std::set container type.
Furthermore, I would suggest to change the default digital set type in StdDefs.h to a hash-map based container to get huge speed-ups in many DGtal and DGtaltools processes.

@dcoeurjo dcoeurjo added this to the 0.9 milestone Jul 17, 2015
@dcoeurjo
Copy link
Member Author

Can someone have a look to this PR ?

@JacquesOlivierLachaud
Copy link
Member

I review it.

define digital sets from any associative container of the STL. For
instance, using std::unordered_set (c++11) or boost::unordered_set (hash
function based containers), speed-up up to 40% can be measured when
processing digital set points. (David Coeurjolly,

Choose a reason for hiding this comment

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

"digital set points" => "digital sets"

@JacquesOlivierLachaud
Copy link
Member

Both compile errors are related to the concept check:
BOOST_STATIC_ASSERT(( boost::is_same<Iterator,ConstIterator>::value ));
This concept check is indeed valid for sorted associative containers, but I wonder why we require it.

@JacquesOlivierLachaud
Copy link
Member

Otherwise there are unrelated errors in Board3D. Was there a problem in the last PR merged ?

@rolanddenis
Copy link
Member

The error in testBoard3D.cpp is due to the insertion of a point outside the domain, line 67 (p6 point). As DigitalSetByAssociativeContainer is now the default DigitalSet for Z2i and Z3i, this class now checks that inserted point are inside the domain, unlike DigitalSetBySTLSet.

@dcoeurjo
Copy link
Member Author

Ok.. I've switched the default DigitalSet as suggested and got plenty of issues which I'm fixing.
So the PR is still a WIP.

BTW, many related issues has been raised because some DigitalSet adapters require more things than expected in the concept. For instance, ImageContainerMap< DigitalSetDomain < DigitalSet > > requires that the set has a reverse iterator which is not the case for DigitalSet models (as described in the concept).

So yes i'm working on that and the PR is more bogus than it was.

@dcoeurjo
Copy link
Member Author

@JacquesOlivierLachaud for your issues, was C++11 activated ?

@dcoeurjo
Copy link
Member Author

(everything compiled perfectly on travis bot --ubuntu-gcc / macos-clang--)

@dcoeurjo
Copy link
Member Author

(when you have backtrace like that, please create a gist and add the link, otherwise the markdown interpretation pollutes everything)

@JacquesOlivierLachaud
Copy link
Member

Yes cpp11 was activated.

@rolanddenis
Copy link
Member

I have the same compilation issues with clang 3.5.0 and C++11 enabled, see https://gist.github.com/rolanddenis/18310385c470a3ac2827

@dcoeurjo
Copy link
Member Author

okay... plenty of concept checks are missing or commented in DGtal... bunch of errors are raised changing the default DigitalSet model. Still working on it

@dcoeurjo
Copy link
Member Author

seems to be ok... @JacquesOlivierLachaud & @rolanddenis can you please check your compilers ?

(FYI, iterator == const_iterator on associative containers is compiler version dependent.. 😢 )

@rolanddenis
Copy link
Member

Everything is fine with my compilers ;)

@dcoeurjo
Copy link
Member Author

cool.. waiting for @JacquesOlivierLachaud 's go and I merge

@JacquesOlivierLachaud
Copy link
Member

It works now on my computer both with clang and g++, both in DEBUG and Release. Good PR since manything will get faster. I merge.

JacquesOlivierLachaud added a commit that referenced this pull request Oct 1, 2015
DigitalSetByAssociativeContainer
@JacquesOlivierLachaud JacquesOlivierLachaud merged commit 03da2c8 into DGtal-team:master Oct 1, 2015
@dcoeurjo dcoeurjo modified the milestones: 0.9, 0.9.2 Jan 24, 2016
@dcoeurjo dcoeurjo deleted the Hashdigitalset branch October 7, 2021 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants