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

Add extra exceptions for arithmetic and types operations #1352

Merged
merged 6 commits into from
Mar 26, 2019

Conversation

copyme
Copy link
Member

@copyme copyme commented Oct 16, 2018

This PR allows for extra exceptions in the debug mode (G++).

So, without these few lines we have all the test passing:

100% tests passed, 0 tests failed out of 193

Total Test time (real) = 448.64 sec

But if we run the tests with the additional exceptions we get:

99% tests passed, 2 tests failed out of 193

Total Test time (real) = 451.77 sec

The following tests FAILED:
54 - testMultiStatistics (Failed)
86 - testLambdaMST3D (NUMERICAL)
Errors while running CTest

The bugs related to LambdaMST3D are already fixed in #1339

It is not that bad only two tests failed (and one of mine). We can think about adding different types of extra exceptions but allowing all may cause problems with boost, for example.

@copyme
Copy link
Member Author

copyme commented Oct 16, 2018

First it looks like there is another flaoting-point error see:

/home/travis/build/DGtal-team/DGtal/tests/dec/testHeatLaplace.cpp:125: FAILED:
due to a fatal error condition:
  SIGFPE - Floating point error signal

Second the thing does not work on OS X. I will have to turn it on only for Linux.

@dcoeurjo
Copy link
Member

dcoeurjo commented Nov 4, 2018

Can you see the issue with testMultiStatistics? I don't know of to trace back the bug..

@copyme
Copy link
Member Author

copyme commented Nov 4, 2018

@dcoeurjo OK, I will try to find a solution once I will have a bit of time.

@dcoeurjo
Copy link
Member

dcoeurjo commented Nov 4, 2018

Thx.. Not easy to locate the actual bug with the SIGFPE signal which does not provide a lot of information

@copyme
Copy link
Member Author

copyme commented Nov 4, 2018

@dcoeurjo Yup, I use GDB to stop at the signal and try to understand where is the problem. I think there are better techniques to deal with this. I will need to have a look at.

@dcoeurjo
Copy link
Member

dcoeurjo commented Nov 4, 2018

I see.. thx anyway..

@dcoeurjo
Copy link
Member

can you please merge with the master ? Are all tests passing now ?

@copyme
Copy link
Member Author

copyme commented Nov 10, 2018

@dcoeurjo They should, lets see what travis will tell us right now.

@copyme
Copy link
Member Author

copyme commented Nov 10, 2018

@dcoeurjo One test is still failing. The fix is in a non-merged branch: Curves3D. I am working on it.

@dcoeurjo dcoeurjo changed the title Add extra exceptions for arithmetic and types operations [WIP] Add extra exceptions for arithmetic and types operations Nov 29, 2018
@dcoeurjo
Copy link
Member

If #1339 is merged, will the remaining test be fine?

@dcoeurjo
Copy link
Member

ping @copyme

@copyme
Copy link
Member Author

copyme commented Mar 25, 2019

@copyme it should be fine I will merge master and we will see

@copyme
Copy link
Member Author

copyme commented Mar 25, 2019

@dcoeurjo sorry for the delay, I have not seen your message. I just added a changelog entry, and if the tests go then you can merge, I think.

@copyme copyme changed the title [WIP] Add extra exceptions for arithmetic and types operations Add extra exceptions for arithmetic and types operations Mar 25, 2019
@copyme
Copy link
Member Author

copyme commented Mar 25, 2019

@dcoeurjo, in fact, there are bugs in the tests. The fix discovered new issues. I will have a look at and try to fix them.

@dcoeurjo
Copy link
Member

👌

@copyme
Copy link
Member Author

copyme commented Mar 25, 2019

@dcoeurjo I will need a hand to fix these issues.

So, in testHeatLaplace.cpp the problem is here:

const double cut_locus = log( - log1p( t ) ) + 2.;
the value of - log1p( t ) is -0.0953102 but std::log does not accept negative values:

If the argument is negative, NaN is returned and FE_INVALID is raised.

Since I do not understand the code well (and I have no time to) I need a piece of precise information on how to fix this issue.

@copyme
Copy link
Member Author

copyme commented Mar 25, 2019

ping @cgurps

@cgurps
Copy link
Contributor

cgurps commented Mar 25, 2019

You can replace this line by

const double cut_locus = 3.0;

The value doesn't really matter for this test

@copyme
Copy link
Member Author

copyme commented Mar 25, 2019

@cgurps thanks, fixed. @dcoeurjo before merging please review my fix to the bug in SphericalHoughNormalVectorEstimator

@copyme
Copy link
Member Author

copyme commented Mar 25, 2019

Also, on my machine I get this:

/home/kacper/Projects/DGtal/cmake-build-debug/tests/dec/testLinearStructure
New Block [testing 3d operators]
  [dec | primal 0-cells <-> dual 3-cells (2) | primal 1-cells <-> dual 2-cells (5) | primal 2-cells <-> dual 1-cells (4) | primal 3-cells <-> dual 0-cells (1)]
  New Block [base operators]
    d0
[primal 0-form => primal 1-form (2 => 5)]
-1  1
 0  1
 0  1
-1  0
-1  0
    d2p
[dual 2-form => dual 3-form (5 => 2)]
-1  0  0 -1 -1
 1  1  1  0  0
    [ERR] Fatal Error - assertion (Eigen::MatrixXd(d0.myContainer) == d0_th) failed in void test_manual_operators_3d(): /home/kacper/Projects/DGtal/tests/dec/testLinearStructure.cpp(391)

Process finished with exit code 134 (interrupted by signal 6: SIGABRT)

But maybe this is Eigen version related since it works well on Travis.

@rolanddenis
Copy link
Member

It is related to recent Boost versions (see #1268).

@copyme
Copy link
Member Author

copyme commented Mar 25, 2019

@rolanddenis thanks for the info!

@copyme
Copy link
Member Author

copyme commented Mar 25, 2019

@dcoeurjo I am done here. Just pay attention to the changes mentioned above while merging.

@dcoeurjo
Copy link
Member

all good.. thanks.

@dcoeurjo dcoeurjo merged commit 6df1c27 into DGtal-team:master Mar 26, 2019
@copyme copyme deleted the FloatingArithm branch March 26, 2019 16:17
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.

4 participants