Add PCL_WARNINGS_ARE_ERRORS CMake option and enable it in Ubuntu 16.04 CI job

[taketwo]

This enables -Wall flag in Ubuntu 16.04 job, which is currently warning-free. The other pipelines still produce warnings; ticket #3379 will continue to track the progress towards having -Wall in all jobs.

[SergioRAgostinho]

ERRORS instead of ERROS. The error is only here, which means the flag was not activated for this build.

[taketwo]

Oh wait, I think we have a problem here. The job failed two times with the same unit test and it does not look like a random failure. I assume this is because I changed the compiler flags.

[SergioRAgostinho]

I'm not really sure what to do in this scenario. I think we'll both agree that changing optimization and warning flags should not produce changes in the outcome of runtime logic (which doesn't run in parallel). But I definitely don't want that unit test rendering CI "useless".

Revert the last commit? The question is also whether you can replicate the failure in your environment.

[taketwo]

The question is also whether you can replicate the failure in your environment.

Let's start with this. Though even if I can, not sure how to fix. Just change the number?

[SergioRAgostinho]

My only idea would be to follow the execution path and try to identify when did both cases diverge and started to produce different results. In C++ this is an extremely time consuming task.

At this moment I prioritize more CI than the GASD feature (11 citations 3 years after publication), so I'm open to just switch numbers.

[SergioRAgostinho]

The gasd test is failing in my environment prior to your commits.

[taketwo]

I think I nailed down the problem. The following lines have a numerical issue:

pcl/features/include/pcl/features/impl/gasd.hpp

Lines 287 to 299 in f36a0d8

	const Eigen::Vector4f p (shape_samples_[i].x, shape_samples_[i].y, shape_samples_[i].z, 0.0f);
	const float d = p.norm ();
	const float shape_grid_step = distance_normalization_factor / shape_half_grid_size_;
	const float dist_hist_start = ((int) (d / shape_grid_step)) * shape_grid_step;
	const float dist_hist_val = (d - dist_hist_start) / shape_grid_step;
	const float dbin = dist_hist_val * shape_hists_size_;
	// add sample to shape histograms, optionally performing interpolation
	addSampleToHistograms (p, max_coord_, shape_half_grid_size_, shape_interp_, dbin, hist_incr_, shape_hists);

With a very unlucky case we get to the following instance:

p =   -0.103523 -0.0401704  0.0485457          0
d = 0.121192
shape_grid_step = 0.0403972
dist_hist_start = 0.121192
dist_hist_val = -1.84433e-07
dbin = -1.84433e-07

The target bin is a tiny negative number instead of zero. Later on, it is rounded and ends up in the bin -1.

I think the fix is to change how dist_hist_val is computed. If you do the math, it's nothing else but the fractional part of d / shape_grid_step, i.e. there is no reason to compute intermediate dist_hist_start variable.

@jpsml

[kunaltyagi]

The target bin is a tiny negative number instead of zero. Later on, it is rounded and ends up in the bin -1

That's a crazy find @taketwo ...

So the fix could be:

float placeholder = d / shape_grid_step;
// can also call modf, but this works if placeholder is within range of long (which it needs to to have a decimal part)
float dist_hist_val = placeholder - static_cast<long int>(placeholder);
// for extra safety incase placeholder > LONG_MAX, dist_hist_val = std::min(1, dist_hist_val);

[taketwo]

That's a crazy find @taketwo ...

Took me quite a few iterations of cout-based debugging on CI to find the issue 😆

Could you explain in more detail why your fix is preferable to modf? I had the following in mind:

float integral;
const float dist_hist_val = std::modf(d / shape_grid_step, &integral);

[kunaltyagi]

Just that modf does a lot of housekeeping and if you know the domain of the variables, a subtraction with long int cast is faster than modf. The checks can't be optimized by compiler because it doesn't know the data domain (in most cases). Analogous to how x*x*x is faster than std::pow(x, 3). Minor concern, but might matter in a tight loop. Your code is ideal in almost all situations though.

[taketwo]

Thanks for the explanation. I kept modf though since it's certainly correct and should not degrade the runtime compared to the old code.

Offtopic: sometimes I wonder, Kunal, where did you get all this knowledge 🕵️‍♂️

[kunaltyagi]

High Performance Computing with Real Time constraints requires every shortcut to reduce compute power, increase IPC. It's a weird combination