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

Debug assert error caused by normal distribution sigma equals 0.0 - MSVC 14.40 #1485

Closed
robbietuk opened this issue Jul 31, 2024 · 1 comment · Fixed by #1486
Closed

Debug assert error caused by normal distribution sigma equals 0.0 - MSVC 14.40 #1485

robbietuk opened this issue Jul 31, 2024 · 1 comment · Fixed by #1486
Labels

Comments

@robbietuk
Copy link
Collaborator

robbietuk commented Jul 31, 2024

Building a cmake project ontop of STIR with CMake (because of #1483).

I am using the example https://github.com/UCL/STIR/blob/master/examples/C%2B%2B/using_installed_STIR/demo_create_image.cxx but building in Debug.

Modifying the program to:

#include <iostream>
#include "stir/Scanner.h"
using namespace std;

int main()
{
	cout << "Here are all the scanner names:\n" << stir::Scanner::list_all_names() << endl;
	return 0;
}

I get an error in C:\Program Files\Microsoft Visual Studio\2022\Professional\VC\Tools\MSVC\14.40.33807\include\random, specifically l:3371

        void _Init(_Ty _Mean0, _Ty _Sigma0) noexcept { // set internal state
            _STL_ASSERT(0.0 < _Sigma0, "invalid sigma argument for normal_distribution");
            _Mean  = _Mean0;
            _Sigma = _Sigma0;
        }

Where the _STL_ASSERT is throwing an exception because 0.0 == _Sigma0. This stems from

distributions[0] = std::normal_distribution<double>(0.0, sigma);

in

protected:
explicit DetectorCoordinateMap(double sigma = 0.0)
: sigma(sigma)
{
# ifdef STIR_OPENMP
generators.resize(omp_get_max_threads());
distributions.resize(omp_get_max_threads());
for (auto& distribution : distributions)
distribution = std::normal_distribution<double>(0.0, sigma);
# else
generators.resize(1);
distributions.resize(1);
distributions[0] = std::normal_distribution<double>(0.0, sigma);
# endif
}

My guess is this hasn't been caught because CI only tests Release on MSVC. This is the only issue I have found so far.


What is the fix?

I am not sure what the logic behind the use of sigma=0.0 to define distributions[0] as a normal distribution with mean=0 and sigma=0 is not a well-defined probability distribution. It essentially collapses to a single point at 0. Likely, the plan was to set distributions[0]=0. Does this seem correct?

@robbietuk robbietuk added the bug label Jul 31, 2024
@robbietuk robbietuk changed the title Debug assert error caused by normal distribution sigmal set to 0.0 with MSVC 14.40 Debug assert error caused by normal distribution sigma equals 0.0 - MSVC 14.40 Jul 31, 2024
@robbietuk
Copy link
Collaborator Author

As far as I can tell, the distributions is providing a way to add noise to the detector coordinates.

stir::CartesianCoordinate3D<float> get_coordinate_for_det_pos(const stir::DetectionPosition<>& det_pos) const
{
auto coord = det_pos_to_coord.at(det_pos);
if (sigma == 0.0)
return coord;
# ifdef STIR_OPENMP
auto thread_id = omp_get_thread_num();
# else
auto thread_id = 0;
# endif
coord.x() += static_cast<float>(distributions[thread_id](generators[thread_id]));
coord.y() += static_cast<float>(distributions[thread_id](generators[thread_id]));
coord.z() += static_cast<float>(distributions[thread_id](generators[thread_id]));
return coord;
}

In its usage, there is a if (sigma == 0.0) return; check. I think the best course of action is to add this to the constructor.

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 a pull request may close this issue.

1 participant