Ticket #8850 (closed: fixed)
Error in SplineWorkspace
Reported by: | Russell Taylor | Owned by: | Vickie Lynch |
---|---|---|---|
Priority: | critical | Milestone: | Release 3.2 |
Component: | Framework | Keywords: | |
Cc: | borreguerojm@… | Blocked By: | |
Blocking: | Tester: | Jose Borreguero |
Description
The following code in SplineWorkspace.cpp (starting at line 88) came to my attention because coverity scan complained about dereferencing a null pointer:
double* yValues(0); m_cspline->derivative1D(yValues, xValues, nData, 2); for(size_t i=0;i<nData;i++) { out->set(i,0,1.); out->set(i,1,xValues[i]); out->set(i,2,yValues[i]); }
yValues needs to be an array, but is not declared as such. The best approach here would be to declare it as a local std::vector and then pass its address to derivative1D.
Also, this class is lacking a unit test, which surely would have picked up this mistake.
Finally, calling a class *Workspace when it's a function is confusing! Could you come up with an alternative name please?
Attachments
Change History
comment:2 Changed 7 years ago by Russell Taylor
- Milestone changed from Release 3.1 to Release 3.2
This work was done for Jose and he's not sure it is doing quite what he wants. He suggests removing the algorithm & function from master for the time being, which will happen in #8877. I won't remove the code, just comment out the registration.
comment:3 Changed 7 years ago by Vickie Lynch
I am only using Roman's CubicSpline interpolation here to convolve spectra from two workspaces. I did not want to make SplineWorkspace a separate function, but Andrei insisted on this when he tested 8761. That is why there is not unit test. I have done testing of convolving two normalized Gaussians to prove that the answer for ConvolveWorkspaces is correct. I will discuss with Jose why this does not do what he wants.
comment:4 Changed 7 years ago by Vickie Lynch
- Status changed from new to inprogress
Refs #8850 remove SplineWorkspace
Changeset: eaad2e997e6102b7a84175b8fd54443d082435a2
comment:5 Changed 7 years ago by Vickie Lynch
Refs #8850 put workspaces into AnalysisDataService for test
Changeset: f3887555c88eda44a332fcadd60d414a53515458
comment:6 Changed 7 years ago by Vickie Lynch
Refs #8850 fix unit test
Changeset: e6619810bfaf891027618ce7c4bcf3c57a094bdc
comment:7 Changed 7 years ago by Vickie Lynch
- Status changed from inprogress to verify
- Resolution set to fixed
To test run ConvolveWorkspaces with attached file for both input workspaces.
comment:8 Changed 7 years ago by Russell Taylor
- Cc borreguerojm@… added
- Tester set to Jose Borreguero
It would be best if Jose tested this ticket.
comment:9 Changed 7 years ago by Jose Borreguero
- Status changed from verify to closed
Merge remote-tracking branch 'origin/feature/8850_SplineWorkspace_error'
Full changeset: 5ada934459a12d9d0920e1a529adae9b18ab8042
comment:10 Changed 7 years ago by Jose Borreguero
Worked as expected
comment:11 Changed 5 years ago by Stuart Campbell
This ticket has been transferred to github issue 9694
There are existing functions CubicSpline or BSpline and also algorithms SplineSmoothing and SplineInterpolation. Is SplineWorkspace a duplicate of one of them?