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.

Sign up for GitHub

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

Jump to bottom

Linearizer #1039

Merged
merged 7 commits into from
Sep 11, 2015
Merged

Linearizer #1039

merged 7 commits into from
Sep 11, 2015

Conversation

rolanddenis
Copy link
Member

It provides linearization of point inside a given HyperRectDomain (with storage order specifier) and the reverse process (index to point). It replaces the linearizer used by ImageContainerBySTLVector and provides additional features:

  • row-major and col-major storage order compatible (maybe not very useful ...);
  • no loops, whatever the dimension ("meta-programming");
  • possibility to be specialized for other domain types (if needed ?).

@rolanddenis rolanddenis mentioned this pull request Sep 8, 2015
Merged
@dcoeurjo
Copy link
Member

dcoeurjo commented Sep 8, 2015

unit test of this class ;) ? For that, you can play with the brand new catchframework to write quick tests.
(it may be difficult since you've removed the old linearizer but I'd love to see performance comparisons..)

dcoeurjo
dcoeurjo reviewed Sep 8, 2015
View reviewed changes
src/DGtal/kernel/domains/Linearizer.h
#endif // else defined(Linearizer_RECURSES)

/* GNU coding style */
/* vim: set ts=2 sw=2 expandtab cindent cinoptions=>4,n-2,{2,^-2,:2,=2,g0,h2,p5,t0,+2,(0,u0,w1,m1 : */
Copy link
Member

Choose a reason for hiding this comment

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

cleanup these editor traces?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oooops, I will clean this !

@dcoeurjo
Copy link
Member

dcoeurjo commented Sep 8, 2015

(changelog entry ?)

@rolanddenis
Copy link
Member Author

Concerning the unit test, I will look at it !
About the speed improvement, I don't think that it will be a big difference because:

  • I think the compiler unrolls the loops of the previous version (since he knows the number of iterations);
  • this version calculates, before linearisation, the difference between the point and the lower bound of the domain, while the previous version calculates it during the linearisation. (is there a speed difference ?)

@dcoeurjo dcoeurjo added this to the 0.9 milestone Sep 9, 2015
@dcoeurjo dcoeurjo added the Image label Sep 9, 2015
@dcoeurjo dcoeurjo self-assigned this Sep 9, 2015
@rolanddenis
Adding test&bench for Linearizer and fixing a bug.
9d06a9e 
The bench is accessible through the test file with hidden tag [.bench]
(see Catch).
@rolanddenis
Copy link
Member Author

Done. I think that it tests enough different cases, don't you ? :D
A benchmark is accessible through hidden tag [.bench], eg:
./tests/kernel/testLinearizer "[.bench]" -d yes
Since each test case (in benchmark) includes a warm-up step, always look at odd lines for timing (corresponding to a section).

Tags are available for each space dimension (up to 5) and each storage order, eg:
./tests/kernel/testLinearizer "[.bench][dim3][ColMajorStorage]" -d yes

@rolanddenis
Forget to add the test file...
b7295b5
@rolanddenis
Trying to fix warnings.
e6c1277 
I can't reproduce gcc warnings because I don't have the same version as
Travis.
Changes in HyperRectDomain_Iterator to avoid negate tests with unsigned
integer in dimension 1.
@rolanddenis
Fixing warnings with gcc (I hope)
77f6b05
dcoeurjo
dcoeurjo reviewed Sep 11, 2015
View reviewed changes
tests/kernel/testLinearizer.cpp
};

#define TEST_LINEARIZER( N , ORDER ) \
TEST_CASE( "Testing Linearizer in dimension " #N " with " #ORDER, "[test][dim" #N "][" #ORDER "]" )\
Copy link
Member

Choose a reason for hiding this comment

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

😄 nice macro based test case generator ;)
unfortunately, there is no such thing in catch yet catchorg/Catch2#358

or catchorg/Catch2#46

Copy link
Member Author

Choose a reason for hiding this comment

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

And there is also a pull request about templated test case: catchorg/Catch2#382
It is maybe possible to do cartesian product with macro ? ;)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I saw this PR too (but stalled since many months)... and for me, macro programming is like juggling with activated grenades.. 😄

@dcoeurjo
Copy link
Member

thanks a lot !
merging

@dcoeurjo dcoeurjo closed this Sep 11, 2015
@dcoeurjo dcoeurjo reopened this Sep 11, 2015
dcoeurjo added a commit that referenced this pull request Sep 11, 2015
@dcoeurjo
Merge pull request #1039 from rolanddenis/pr_linearizer
50b7fab 
Linearizer
@dcoeurjo dcoeurjo merged commit 50b7fab into DGtal-team:master Sep 11, 2015
@rolanddenis rolanddenis deleted the pr_linearizer branch October 16, 2015 15:33
@dcoeurjo dcoeurjo modified the milestones: 0.9, 0.9.2 Jan 24, 2016
@dcoeurjo dcoeurjo modified the milestones: 0.9.2, 0.9 Jan 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@dcoeurjo dcoeurjo

Labels
Image waiting author feedbacks
Projects
None yet
Milestone
0.9.2
Development

Successfully merging this pull request may close these issues.
2 participants
@rolanddenis @dcoeurjo