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

Fixing arithmetic ring issue #1163

Merged
merged 11 commits into from
May 8, 2016
Merged

Conversation

jlevallois
Copy link
Member

@jlevallois jlevallois commented May 3, 2016

PR Description

The aim of this PR is to remove arithmetic ring issue when we change the integer type of SpaceND/KhalimskySpaceND.
By convention, we use SpaceND<dim, int32_t> and KhalimskySpaceND<dim, int32_t>.
But, in some case (if we want to reduce the memory footprint for example), we could be led to use int16_t instead of int32_t (or other).
The issue is that some algorithms are hard-coded to use int32_t. This PR try to resolve this issue.

(I'm sorry, the reviewing process will not be easy because my editor has deleted some unnecessary space in comments ...)

Critical files modified

These files need to be closely reviewed.

  • AlphaThickSegmentComputer.h
  • PowerMap.h / PowerMap.ih
  • VoronoiMap.h / VoronoiMap.ih
  • RigidTransformation3D.h
  • Display2DFactory.ih
  • DrawWithDisplay3DModifier.h
  • BasicPointFunctors.h

Test files modified

  • testAlphaThickSegmentComputer.cpp
  • testFrechetShortcut.cpp
  • testFreemanChain.cpp
  • testDistanceTransformationND.cpp
  • testImageSimple.cpp
  • testImageSpanIterators.cpp
  • testSliceImageFromFunctor.cpp
  • testDigitalSet.cpp
  • testHyperRectDomain-snippet.cpp
  • testHyperRectDomain.cpp
  • testCellularGridSpaceND.cpp
  • testSurfaceHelper.cpp

Checklist

  • [N/A] Unit-test of your feature with Catch.
  • [N/A] Doxygen documentation of the code completed (classes, methods, types, members...)
  • [N/A] 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)

:octocat:

typedef typename InputPointContainer::iterator Iterator;
typedef TConstIterator ConstIterator;
typedef ParallelStrip< SpaceND< 2, DGtal::int32_t > ,true,true> Primitive;

typedef ParallelStrip< SpaceND< 2 > ,true,true> Primitive;
Copy link
Member

Choose a reason for hiding this comment

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

why did you change this one ?
Even if int32_t is already the default type for SpacND, maybe the author as a good reason to want a int32_t type (if for instance we change the default type in SpaceND

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, in this case I need to change testAlphaThickSgementComputer who uses Z2i::Point/Z2i::Domain

Copy link
Member

@dcoeurjo dcoeurjo May 4, 2016

Choose a reason for hiding this comment

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

No I think it should be SpaceND<2, InputPoint::Coordinate> here

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't, because of AlphaThickSegmentComputer< Z2i::RealPoint> in testAlphaThickSgementComputer (line 199)

Copy link
Member

Choose a reason for hiding this comment

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

Ok I'll have a look.

Copy link
Member Author

Choose a reason for hiding this comment

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

It works when hardcoding ParallelStrip<SpaceND< 2, int32_t>, ...> because his operator() has a surcharge for Point and RealPoint (always double) from SpaceND.

Copy link
Member

Choose a reason for hiding this comment

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

Ok... And there is no implicit cast double -> int when used ?

Copy link
Member Author

Choose a reason for hiding this comment

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

double doesn't follow the concept CInteger, require by SpaceND

Copy link
Member

Choose a reason for hiding this comment

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

Yeah of course. I meant in the code, are input points casted to "primitive" points at some places (which would mean an implicit cast if input points are RealPoints)

@dcoeurjo
Copy link
Member

dcoeurjo commented May 4, 2016

nice PR BTW

@dcoeurjo
Copy link
Member

dcoeurjo commented May 8, 2016

thanks again @jlevallois
Can you please revert your edit in Alpha... (reset the int32_t template type) and add an entry to the changeling ?
Beside, the PR is fine, thanks again

@jlevallois
Copy link
Member Author

jlevallois commented May 8, 2016

It's done :)

:octocat:

@dcoeurjo
Copy link
Member

dcoeurjo commented May 8, 2016

Thanks Merging

@dcoeurjo dcoeurjo merged commit 5ee586a into DGtal-team:master May 8, 2016
@jlevallois jlevallois deleted the FixArithmeticRing branch May 8, 2016 15:26
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.

2 participants