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

(Reverse) Distance Transformation on Periodic Spaces #1206

Merged
merged 55 commits into from
Nov 4, 2016

Conversation

rolanddenis
Copy link
Member

@rolanddenis rolanddenis commented Sep 8, 2016

PR Description

Adding per-dimension periodicity specification to VoronoiMap (+DistanceTransformation) and PowerMap (+ReverseDistanceTransformation), following D. Coeurjolly, 2008, Distance Transformation, Reverse Distance Transformation and Discrete Medial Axis on Toric Spaces 😲

Checklist

  • Adding per-dimension periodicity specification
    • VoronoiMap & DistanceTransformation
    • PowerMap & ReverseDistanceTransformation
  • Adding methods to get bounded coordinates of nearest site.
  • Unit-test of your feature with Catch.
    • VoronoiMap & DistanceTransformation(partial)
    • PowerMap & ReverseDistanceTransformation(partial) & ReducedMedialAxis
  • Doxygen documentation of the code completed (classes, methods, types, members...)
    • VoronoiMap & DistanceTransformation
    • PowerMap & ReverseDistanceTransformation
  • Documentation module page added or updated.
    • nD Volumetric Analysis using Separable Processes (moduleVolumetric)
    • [ ] Tutorial "Image -> Region -> Distance Transformation" (tutoImageRegionDT)
  • 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)

@rolanddenis rolanddenis changed the title (Reverse) Distance Transformation on Toric Spaces (Reverse) Distance Transformation on Periodic Spaces Sep 8, 2016
@dcoeurjo dcoeurjo added this to the 0.9.3 milestone Sep 30, 2016
@dcoeurjo dcoeurjo self-assigned this Sep 30, 2016
@dcoeurjo
Copy link
Member

sorry for the delay @rolanddenis .

What's wrong with the break index in power map?

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.

request changes
(still playing;))

// In the periodic case, the cycle depends on break index
if ( isPeriodic(dim) )
{
// Along other than the first dimension, the break index is at the lower site found.
Copy link
Member

Choose a reason for hiding this comment

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

lowest

(just playing with new Github review system)

Copy link
Member Author

Choose a reason for hiding this comment

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

Hum ... and now ? Do you have to approve the changes or did I miss something ?

Copy link
Member

Choose a reason for hiding this comment

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

not clear..

@dcoeurjo
Copy link
Member

dcoeurjo commented Oct 20, 2016

(your HDF5 fix does not work neither on my mac, I've no idea what the problem is :()

@rolanddenis
Copy link
Member Author

😰 I was trying the solution from http://stackoverflow.com/questions/34050155/symbol-not-found-linking-to-hdf-library ... Even if it doesn't solve the problem, it seems that requiring the CXX component of HDF5 is not a bad idea (but I will revert the commit, it is not the right PR for this).

So you also have the error on your computer ? It is not just a Travis specific configuration problem...

@rolanddenis
Copy link
Member Author

I think the problem is related to that : http://stackoverflow.com/questions/16352833/linking-with-clang-on-os-x-generates-lots-of-symbol-not-found-errors

It may come from the fact that hdf5 has been compiled with a C++ library different to the one used to compile DGtal. I also had the same problem when linking a program compiled with gcc >=5 with boost compiled with gcc < 5 : the std::basic_string symbol changed with gcc 5 (like the error in Travis).

Another link on this problem: http://stackoverflow.com/questions/8454329/why-cant-clang-with-libc-in-c0x-mode-link-this-boostprogram-options-examp

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.

By default, brew use clang compiler (same one as DGTal). I think the problem is more related to hdf5 upstream issue rather that something from our side

@dcoeurjo
Copy link
Member

(And I agree, CXX flag for hdf5 was missing)

@rolanddenis
Copy link
Member Author

Yes, I agree...

@dcoeurjo
Copy link
Member

dcoeurjo commented Nov 1, 2016

thanks a lot @rolanddenis for your edits. I'm reviewing them.

@dcoeurjo
Copy link
Member

dcoeurjo commented Nov 1, 2016

all bots are fine now. @rolanddenis can you please add something in the doc about the "(partial)" thing?
thx

@rolanddenis
Copy link
Member Author

I added a small paragraph on partial periodic specification (even if your remark was not about that) ;)

@dcoeurjo
Copy link
Member

dcoeurjo commented Nov 4, 2016

thanks.

@dcoeurjo
Copy link
Member

dcoeurjo commented Nov 4, 2016

Great merging. thanks again

@dcoeurjo dcoeurjo closed this Nov 4, 2016
@dcoeurjo dcoeurjo reopened this Nov 4, 2016
@dcoeurjo dcoeurjo merged commit 38cc1eb into DGtal-team:master Nov 4, 2016
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.

2 participants