Ticket #9682 (closed: fixed)
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: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