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

General update and switch to std::mutex #27

Merged
merged 6 commits into from
Oct 29, 2018
Merged

General update and switch to std::mutex #27

merged 6 commits into from
Oct 29, 2018

Conversation

dzenanz
Copy link
Member

@dzenanz dzenanz commented Oct 22, 2018

I was getting some errors with this. An independent test/update of this PR would be welcome.

@thewtex thewtex mentioned this pull request Oct 22, 2018
Merged
@dzenanz
Copy link
Member Author

dzenanz commented Oct 23, 2018

As there are other pull requests, maybe we should wait until they are merged or closed before trying to fix this PR? Rebasing on top of other fixes might help.

@jhlegarreta
Copy link
Member

Sounds like a good plan.

@dzenanz dzenanz mentioned this pull request Oct 25, 2018
Closed
dzenanz and others added 2 commits October 25, 2018 15:18
@dzenanz
Revert "COMP: Use C++11 ITK_NOEXCEPT directly"
266da02 
This reverts commit b0ea16b.

C++ headers were included from a C source file. This commit fixes the following error on Visual Studio 2017 version 15.8.8:

1>------ Build started: Project: VariationalRegistration, Configuration: Debug x64 ------
1>VariationalRegistrationMain.cxx
1>getopt.c
1>c:\program files (x86)\microsoft visual studio\2017\community\vc\tools\msvc\14.15.26726\include\cstdio(29): error C2061: syntax error: identifier 'std'
1>c:\program files (x86)\microsoft visual studio\2017\community\vc\tools\msvc\14.15.26726\include\cstdio(29): error C2059: syntax error: ';'
1>c:\program files (x86)\microsoft visual studio\2017\community\vc\tools\msvc\14.15.26726\include\cstdio(29): error C2449: found '{' at file scope (missing function header?)
1>c:\program files (x86)\microsoft visual studio\2017\community\vc\tools\msvc\14.15.26726\include\cstdio(56): error C2059: syntax error: '}'
1>c:\dev\itk-git\modules\remote\variationalregistration\src\win32_compatibility\getopt.c(123): fatal error C1004: unexpected end-of-file found
1>Done building project "VariationalRegistration.vcxproj" -- FAILED.
========== Build: 0 succeeded, 1 failed, 48 up-to-date, 0 skipped ==========
@jhlegarreta @dzenanz
ENH: Adapt to new multi-threading naming convention.
9ce8a7c 
Adapt to new multi-threading naming convention. Specifically:
- Change `MultiThreader::ThreadInfoStruct` to
  `MultiThreaderBase::WorkUnitInfo`
- Change `(threadInfo)->ThreadID` to `(threadInfo)->WorkUnitID`
- Change `(threadInfo)->NumberOfThreads` to
  `(threadInfo)->NumberOfWorkUnits`

after commit:
InsightSoftwareConsortium/ITK@ce15429

Addresses deprecation warnings:
```
itkvariationalregistration\include\itkVariationalRegistrationDiffusionRegularizer.hxx(238):
warning C4996: 'itk::MultiThreaderBase::ThreadInfoStruct': Use
WorkUnitInfo, ThreadInfoStruct is deprecated since ITK 5.0
```
@dzenanz
Copy link
Member Author

dzenanz commented Oct 25, 2018

On my computer, 30% tests passed, 14 tests failed out of 20. All 2D tests need an update to baseline images. 3D tests have some weird artifacts around the edges of images, so we shouldn't just take the current outputs as baselines.

@dzenanz dzenanz changed the title WIP: General update and switch to std::mutex General update and switch to std::mutex Oct 25, 2018
@dzenanz
Copy link
Member Author

dzenanz commented Oct 26, 2018

Could we merge this as is? It is an improvement, and there are plenty of things to do to fix this completely.

@thewtex
Copy link
Member

thewtex commented Oct 26, 2018

We should try to keep CI green, if possible. There are build errors:

In file included from /ITKVariationalRegistration/include/itkVariationalRegistrationDiffusionRegularizer.h:191:0,
                 from /ITKVariationalRegistration/include/itkVariationalRegistrationFilter.h:26,
                 from /ITKVariationalRegistration/test/VariationalRegistrationFilterTest.cxx:19:
/ITKVariationalRegistration/include/itkVariationalRegistrationDiffusionRegularizer.hxx: In static member function 'static void* itk::VariationalRegistrationDiffusionRegularizer<TDisplacementField>::CalcBufferCallback(void*)':
/ITKVariationalRegistration/include/itkVariationalRegistrationDiffusionRegularizer.hxx:270:10: error: 'ITK_THREAD_RETURN_DEFAULT_VALUE' was not declared in this scope
   return ITK_THREAD_RETURN_DEFAULT_VALUE;

@dzenanz
Copy link
Member Author

dzenanz commented Oct 26, 2018

That was due to a change Hans did not long ago. The CI infrastructure should be updated to use a newer commit. Do you want me to do it, or will you do it yourself @thewtex?

@thewtex
Copy link
Member

thewtex commented Oct 26, 2018

That was due to a change Hans did not long ago. The CI infrastructure should be updated to use a newer commit. Do you want me to do it, or will you do it yourself @thewtex?

@dzenanz please go ahead.

@dzenanz
Copy link
Member Author

dzenanz commented Oct 29, 2018

65% tests passed, 7 tests failed out of 20

Label Time Summary:
VariationalRegistration = 241.04 sec*proc (20 tests)

Total Test time (real) = 155.20 sec

The following tests FAILED:
6 - VariationalRegistrationGaussian2DTest (Failed)
15 - VariationalRegistrationGaussian3DTest (Failed)
16 - VariationalRegistrationDiffusive3DTest (Failed)
17 - VariationalRegistrationSSD3DTest (Failed)
18 - VariationalRegistrationNCC3DTest (Failed)
19 - VariationalRegistrationDiffeomorph3DTest (Failed)
20 - VariationalRegistrationSymDiff3DTest (Failed)

@dzenanz
Copy link
Member Author

dzenanz commented Oct 29, 2018

Should we merge this as is? It is big improvement.

@hjmjohnson
Copy link
Member

@dzenanz I'd suggest that this the failing test be commented out so that the dashboard is green and make an issue to fix the rest of the tests.

@dzenanz dzenanz mentioned this pull request Oct 29, 2018
Open
@dzenanz
Copy link
Member Author

dzenanz commented Oct 29, 2018

All the 3D tests are failing, I think. And one 2D test is crashing. I would rather leave that painfully obvious then temporarily sweep it under the carpet.

@thewtex
Copy link
Member

thewtex commented Oct 29, 2018

Please keep master in a working state.

@dzenanz
Copy link
Member Author

dzenanz commented Oct 29, 2018

@thewtex Master is not in a working state, as can be seen on top of the README.rst. Master doesn't even compile, and this PR will bring it closer to working (65% tests passed).

@thewtex
Copy link
Member

thewtex commented Oct 29, 2018

@thewtex Master is not in a working state, as can be seen on top of the README.rst. Master doesn't even compile, and this PR will bring it closer to working (65% tests passed).

Ok, then thanks for moving the builds forward. 👍

Please see inline comment regarding this->DynamicMultiThreadingOff(). If it does not apply, then please go ahead and merge.

@dzenanz dzenanz merged commit 33acbe7 into InsightSoftwareConsortium:master Oct 29, 2018
@jhlegarreta jhlegarreta mentioned this pull request Nov 4, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhlegarreta jhlegarreta jhlegarreta left review comments

@thewtex thewtex thewtex left review comments

@hjmjohnson hjmjohnson Awaiting requested review from hjmjohnson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dzenanz @jhlegarreta @thewtex @hjmjohnson