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

Updated statismo to use C++11 #268

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

muefra
Copy link

@muefra muefra commented Jan 24, 2018

  • Mostly removed boost (it is still used for the CLI tools)
  • Updated CMake files to reflect those changes
  • Superbuild update:
    • Removed git as a dependency: speeds up source downloads
    • Update of dependencies (HDF5 is still on 1.8.X, support for 1.10 will be in a separate pull request)
  • Added ability to statically initialize PRNG (more reliable unit tests)
  • Fixed 1 Unit test (switched to relative error instead of absolute error when comparing floats to account for limited numerical accuracy)

- Mostly removed boost (it is still used for the CLI tools)
- Updated CMake files to reflect those changes
- Superbuild update (Removed git as a dependency: speedup in source download)
- Added ability to statically initialize PRNG (more reliable unit tests)
@muefra
Copy link
Author

muefra commented Jan 24, 2018

Seems that the ITK bundled with ubuntu trusty is a bit old and doesn't like C++11 (Didin't notice this prior to the PR because I tested it on ubuntu xenial)

I'll look into this in the evening.

@muefra
Copy link
Author

muefra commented Jan 24, 2018

Well, as it turns out ubuntu 14.04 is a bit old (eg: per default cmake ist still on version 2.8; interestingly enough the travis image seems to come with a newer cmake version though).

Got it to work regardless, although using -fpermissive for gcc to swallow the old dependencies of the old itk version isn't particularly pretty. Using the superbuild on travis would be a bit slow, so it'll have to suffice.

I also added make test to run to unit tests after the build.

@marcelluethi
Copy link
Member

@muefra Thank you very much for this very extensive set of changes. It will greatly simplify things. As there are so many changes, it might take a week or two before I can have a careful look.

@normanius
Copy link
Contributor

normanius commented Apr 27, 2018

Looks okay for me on MacOS with clang++ and even ccache+clang++. The only thing I noticed are some warning messages arising from ITK. For example:

In file included from ../modules/ITK/examples/itkBuildGaussianProcessDeformationModel.cxx:46:
../modules/ITK/include/itkStandardImageRepresenter.h:151:24: warning: 'CloneDataset' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
    DatasetPointerType CloneDataset(DatasetConstPointerType d) const;
                       ^
../modules/core/include/Representer.h:190:32: note: overridden virtual function is here
    virtual DatasetPointerType CloneDataset(DatasetConstPointerType d) const = 0;
                               ^

I think this is only related to ITK and not to statismo. One could silence this by adding a line add_compile_options("-Wno-inconsistent-missing-override") in the main CMakeLists. I didn't find a way to silence only the ITK-related warnings. (include_directories(SYSTEM ${ITK_INCLUDE_DIRS} ) didn't work for me.

Suggested fix:

if( ${ITK_SUPPORT} MATCHES "ON" )
  find_package( ITK REQUIRED PATHS ${INSTALL_DEPENDENCIES_DIR})
  include( ${ITK_USE_FILE} )
  #include_directories(SYSTEM ${ITK_INCLUDE_DIRS} )
  add_compile_options("-Wno-inconsistent-missing-override")
endif()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants