Commit 9918c93b authored by Danny Hindson's avatar Danny Hindson
Browse files

Fix access violation in MonteCarloAbsorption

The unit test suite MonteCarloAbsorptionTest has occasionally been failing
with an access violation on Jenkins. I've been able to track this down to
the test Lambda_StepSize_Two_Linear_Interpolation which was failing

I can reproduce this locally by modifying the test to repeat 1000 times
and it eventually fails. Although the access violation occurs in a couple
of different places:
a) the y() or e() vector in the 0th histogram is sometimes empty once the
MonteCarloAbsorption algorithm completes which causes some of the test asserts
to fall over because they assume y(0).front() or e(0).front
b) sometimes the code in MonteCarloAbsorption itself has an access violation
when yIndexOfX returns an index out of range of the histogram

Both problems happen in the multi-threaded code in MonteCarloAbsorption and
it seems to be the interaction between the call to MatrixWorkspace::yIndexOfX
and the code that updates a histogram with interpolated results via
MatrixWorkspace::setHistogram. Something in these two functions isn't threadsafe
I found that yIndexOfX calls IsHistogramData() which checks the 0th
histogram in the workspcae even if the MonteCarloAbsorption code is
processing the 1st, 2nd etc workspace
I think if the 0th histogram is in the process of being updated by another
thread then yIndexOfX can fail or else the setHistogram call fails.
I have done two changes:
- only call yIndexOfX once (there was a redundant 2nd call)
- I have modified yIndexOfX so it takes an additional histogram index parameter.
This allows me to keep each MonteCarloAbsorption thread interacting with a
single histogram only. I have defaulted the new parameter to zero so other
code behaves as before
parent 257cb38b
......@@ -423,7 +423,7 @@ public:
/// Returns true if the workspace contains data in histogram form (as
/// opposed to point-like)
virtual bool isHistogramData() const;
virtual bool isHistogramData(std::size_t index = 0) const;
/// Returns true if the workspace contains common X bins
virtual bool isCommonBins() const;
......
......@@ -1051,18 +1051,19 @@ void MatrixWorkspace::setDistribution(bool newValue) {
* Whether the workspace contains histogram data
* @return whether the workspace contains histogram data
*/
bool MatrixWorkspace::isHistogramData() const {
bool MatrixWorkspace::isHistogramData(const std::size_t index) const {
// all spectra *should* have the same behavior
bool isHist = (x(0).size() != y(0).size());
bool isHist = (x(index).size() != y(index).size());
// TODOHIST temporary sanity check
if (isHist) {
if (getSpectrum(0).histogram().xMode() !=
if (getSpectrum(index).histogram().xMode() !=
HistogramData::Histogram::XMode::BinEdges) {
throw std::logic_error("In MatrixWorkspace::isHistogramData(): "
"Histogram::Xmode is not BinEdges");
}
} else {
if (getSpectrum(0).histogram().xMode() !=
if (getSpectrum(index).histogram().xMode() !=
HistogramData::Histogram::XMode::Points) {
throw std::logic_error("In MatrixWorkspace::isHistogramData(): "
"Histogram::Xmode is not Points");
......@@ -1385,7 +1386,7 @@ std::size_t MatrixWorkspace::yIndexOfX(const double xValue,
throw std::out_of_range("MatrixWorkspace::yIndexOfX - X value is greater"
" than the highest in the current range.");
if (this->isHistogramData())
if (isHistogramData(index))
return binIndexOfValue(xValues, xValue, ascendingOrder, tolerance);
else
return xIndexOfValue(xValues, xValue, tolerance);
......
......@@ -401,13 +401,9 @@ MatrixWorkspace_uptr MonteCarloAbsorption::doSimulation(
}
for (size_t j = 0; j < packedLambdas.size(); j++) {
simulationWS.getSpectrum(i)
.dataY()[simulationWS.yIndexOfX(packedLambdas[j], i)] =
packedAttFactors[j];
simulationWS.getSpectrum(i)
.dataE()[simulationWS.yIndexOfX(packedLambdas[j], i)] =
packedAttFactorErrors[j];
auto idx = simulationWS.yIndexOfX(packedLambdas[j], i);
simulationWS.getSpectrum(i).dataY()[idx] = packedAttFactors[j];
simulationWS.getSpectrum(i).dataE()[idx] = packedAttFactorErrors[j];
}
// Interpolate through points not simulated. Simulation WS only has
......
......@@ -137,7 +137,7 @@ public:
// Returns true always - an EventWorkspace always represents histogramm-able
// data
bool isHistogramData() const override;
bool isHistogramData(std::size_t index) const override;
std::size_t MRUSize() const;
......
......@@ -452,7 +452,7 @@ void EventWorkspace::switchEventType(const Mantid::API::EventType type) {
/// Returns true always - an EventWorkspace always represents histogramm-able
/// data
/// @returns If the data is a histogram - always true for an eventWorkspace
bool EventWorkspace::isHistogramData() const { return true; }
bool EventWorkspace::isHistogramData(std::size_t index) const { return true; }
/** Return how many entries in the Y MRU list are used.
* Only used in tests. It only returns the 0-th MRU list size.
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment