Ticket #9682 (closed: fixed)

Opened 6 years ago

Last modified 5 years ago

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

Reported by: Michael Wedel Owned by: Michael Wedel
Priority: major Milestone: Release 3.3
Component: Framework Keywords:
Cc: Blocked By:
Blocking: Tester: Martyn Gigg

Description (last modified by Michael Wedel) (diff)

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.

Change History

comment:1 Changed 6 years ago by Michael Wedel

  • Description modified (diff)

comment:2 Changed 6 years ago by Michael Wedel

  • Description modified (diff)

comment:3 Changed 6 years ago by 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

comment:4 Changed 6 years ago by 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.

comment:5 Changed 6 years ago by 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

comment:6 Changed 6 years ago by 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.

comment:7 Changed 6 years ago by Michael Wedel

  • Owner set to Michael Wedel
  • Status changed from new to assigned
  • Milestone changed from Backlog to Release 3.3

comment:8 Changed 6 years ago by Michael Wedel

  • Status changed from assigned to inprogress

Re #9682. Implemented binary search

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

Changeset: ce9f979652762e124c5c942ca0b28cbdcbe4520b

comment:9 Changed 6 years ago by Michael Wedel

  • Summary changed from Lookup table is filled twice when instrument is loaded to Refactor Kernel::Interpolation to correctly handle multiple loading from string

comment:10 Changed 6 years ago by Michael Wedel

Re #9682. Fix initializer list in unit test.

Changeset: 625079cbe6c48ceadfb6d606f78c48d688c39b4c

comment:11 Changed 6 years ago by 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".

comment:12 Changed 6 years ago by Michael Wedel

  • Status changed from inprogress to verify
  • Resolution set to fixed

comment:13 Changed 6 years ago by Michael Wedel

  • Status changed from verify to reopened
  • Resolution fixed deleted

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.

comment:14 Changed 6 years ago by Michael Wedel

  • Status changed from reopened to inprogress

Re #9682. Handling case with one interpolation value.

Changeset: 167c634adf51144b0ea2d340bae149dd24ec6a9b

comment:15 Changed 6 years ago by 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

comment:16 Changed 6 years ago by Michael Wedel

  • Status changed from inprogress to verify
  • Resolution set to fixed

comment:17 Changed 6 years ago by Owen Arnold

  • Status changed from verify to verifying
  • Tester set to Owen Arnold

comment:18 Changed 6 years ago by Owen Arnold

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

comment:19 Changed 6 years ago by Owen Arnold

  • Status changed from verifying to verify
  • Tester Owen Arnold deleted

comment:20 Changed 6 years ago by Wenduo Zhou

  • Status changed from verify to verifying
  • Tester set to Wenduo Zhou

comment:21 Changed 6 years ago by Wenduo Zhou

  • Status changed from verifying to verify
  • Tester Wenduo Zhou deleted

comment:22 Changed 6 years ago by Martyn Gigg

  • Status changed from verify to verifying
  • Tester set to Martyn Gigg

comment:23 Changed 6 years ago by Martyn Gigg

  • Status changed from verifying to closed

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.

comment:24 Changed 6 years ago by Martyn Gigg

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

Full changeset: 3dd1903072fe8eb634c70a1cc5a63a8a016541fb

comment:25 Changed 5 years ago by Stuart Campbell

This ticket has been transferred to github issue 10524

Note: See TracTickets for help on using tickets.