Refactor Kernel::Interpolation to correctly handle multiple loading from string

[Michael Wedel]

When an instrument definition contains a FitParameter with a lookup-table, all values are inserted into the table twice (in some situations even 4 times).

I have traced this down to LoadInstrument::exec():

174      // populate parameter map of workspace 
175      m_workspace->populateInstrumentParameters();
176
177      // check if default parameter file is also present, unless loading from
178      if (!m_filename.empty())
179        runLoadParameterFile();

Both line 175 and 179 execute the >> operator for Interpolation (eventually), on the same instance, so the data stored in the IDF are added twice.

Adding a clearData() method to Interpolation and calling that inside the operator fixes the problem, but maybe it's possible to track down the real origin of this behavior.

If anyone knows details of this loading procedure, I would be happy about advice.

[Michael Wedel]

Re #9682. Added resetData method to Interpolation

Cleaned up the unit tests a bit so that it's easier to spot where errors are coming from.

Changeset: a4517162b96cb89beb613663e3206abb91b14efa

[Michael Wedel]

The unit test "testStreamOperatorsNonEmpty" fails on the old code and passes with the suggested data-reset-method.

In summary, having each data pair twice in the table makes the interpolation results incorrect outside the interpolation range, because the slope of the first two values is always zero (since the values are identical), the same is true for the last two values.

Results at the limits are incorrect as well, as they are handled in the same way.

The "bulk" values inside the interpolation range are not affected.

[Michael Wedel]

Re #9682. Expanded unit tests for Interpolation

The edge-cases of an interpolation are now clearly separated and checked in a uniform way.

Changeset: 1804557f30cb21b6d7293b9a010c30fd744be79e

[Michael Wedel]

While spending some time looking at the code that performs the interpolation itself, I noticed that linear search is used to find the "nearest neighbor" in Interpolation::value(). Since the internal value list is always kept in sorted state, it could be an option to implement binary search instead.

[Michael Wedel]

Re #9682. Implemented binary search

Replaced the linear search with a binary search and added unit tests.

Changeset: ce9f979652762e124c5c942ca0b28cbdcbe4520b

[Michael Wedel]

Re #9682. Fix initializer list in unit test.

Changeset: 625079cbe6c48ceadfb6d606f78c48d688c39b4c

[Michael Wedel]

Testing information:

Code review, making sure all tests pass.

Finally, I am not 100% sure if my idea of "correct behavior" is actually what was intended. I guess it could be possible that a lookup-table defined in the IDF is extended through the parameter file, but replacement of the units and no checks for that in the stream operator lead me to the conclusion that the intended behavior is "replacment of interpolation values".

[Michael Wedel]

Valgrind detected a memory problem caused by the copy constructor test. The error occurs when there is only one value in the interpolation, which should be handled like the 0-values case.

[Michael Wedel]

Re #9682. Handling case with one interpolation value.

Changeset: 167c634adf51144b0ea2d340bae149dd24ec6a9b

[Michael Wedel]

Re #9682. Changed behavior of interpolation with one value

In the previous commit, I fixed the problem caused by having only one value in the interpolation table by return 0. On second thought, it makes more sense to assume a "constant interpolation" and return the one stored y-value for any x.

Changeset: 2e6276269c0c5e33cf08c544fac3bd5b1e2606d7

[Owen Arnold]

Not for 3.2 (my mistake) so putting back into the testing pool till after the release.

[Martyn Gigg]

After looking at the code in more detail I think the duplication has actually already been solved by #9802. The duplication was actually caused but the ParameterMap reusing the same Interpolation object when adding a new object rather than creating a new one. The old code looked like this: ​https://github.com/mantidproject/mantid/blob/1669e65dc8194a70978468ac2063b372bd4bc502/Code/Mantid/Framework/Geometry/src/Instrument/ParameterMap.cpp#L235

In #9802 changes were made, for a different reason, so that adding a new value would always create a brand new object and I think this solved the problem.

Despite this, I still believe the changes in this ticket are worth making as the way the Interpolation operator>> behaves does suggest that it expects a full clear each time.

[Martyn Gigg]

Merge remote-tracking branch 'origin/feature/9682_lookup_table_read_multiple_times'

Full changeset: ​3dd1903072fe8eb634c70a1cc5a63a8a016541fb

[Stuart Campbell]

This ticket has been transferred to github issue ​10524