Ticket #10996 (closed: fixed)
Refactor PoldiSpectrumDomainFunction to use arbitrary profile functions
Reported by: | Michael Wedel | Owned by: | Michael Wedel |
---|---|---|---|
Priority: | major | Milestone: | Release 3.4 |
Component: | Diffraction | Keywords: | POLDI |
Cc: | Blocked By: | #11104 | |
Blocking: | Tester: | Federico M Pouzols |
Description
In its current form with all the parameter transformations it will be very hard (or impossible) to replace the profile function that is used in PoldiSpectrumDomainFunction. After some experiments I came to the conclusion that there's an equivalent way of calculation the function that does not require any parameter transformation. This means that PoldiSpectrumDomainFunction can be seen as a wrapper for an IPeakFunction, which makes it possible to finally use other profile functions and easily add others (which are then general and not limited to POLDI).
In the scope of this ticket I would like to implement those changes.
Change History
comment:2 Changed 6 years ago by Michael Wedel
Refs #10996. Removing comments, debug output, adjusting unit test.
Changeset: 0b60fd5b9f73903c94d458c52cf133752b4bf635
comment:3 Changed 6 years ago by Michael Wedel
Refs #10996. Checkpointing work.
Changeset: b08cc36603edf966c75ad25d87e0496eb57e5295
comment:4 Changed 6 years ago by Michael Wedel
Refs #10996. Added test function (area based gaussian)
Changeset: b64ae3331156ad29d6011a1a651abb22b010f5fc
comment:5 Changed 6 years ago by Michael Wedel
Refs #10996. Adding local jacobian to handle derivatives.
Changeset: 4e10784fcfe66a6d9e42a1503d105fc568e43224
comment:8 Changed 6 years ago by Michael Wedel
- Status changed from assigned to inprogress
Refs #10996. Format correction
Changeset: 0d6e5dd35ddd7c687a9c5cda3faa2d626106adb6
comment:9 Changed 6 years ago by Michael Wedel
Refs #10996. factoring out profile function
Changeset: 0fdf943206d55b0752647b3765347542a59b290d
comment:10 Changed 6 years ago by Michael Wedel
Refs #10996. Removing AreaGaussian
This is no longer required and is removed.
Changeset: 6b88ed7fdc0a2310fe60bde3b58ff21d36c7f993
comment:11 Changed 6 years ago by Michael Wedel
Refs #10996. Removing debugging statements
Changeset: 2c91349ad7de58a6667d49fcfc116c5fd75bf96f
comment:12 Changed 6 years ago by Michael Wedel
Refs #10996. Fixing PoldiFitPeaks2DTest
Changeset: c75cfb170d01bc20bb665eda68c89a97f13fc490
comment:13 Changed 6 years ago by Michael Wedel
Refs #10996. Remove actualFunction from PoldiSpectrumDomainFunction
Changeset: d0268f96eef1422abf57e38899945eff318aeefd
comment:14 Changed 6 years ago by Michael Wedel
Refs #10996. Fix PoldiSpectrumDomainFunctionTest
Changeset: bd126a3c35f29b5f71910c931f548856249d1a14
comment:15 Changed 6 years ago by Michael Wedel
Replacing peak integration with new intensity methods of IPeakFunction
Refs #10996.
Changeset: b17e83d92780c9834f6048cd15d0f86b40177465
comment:16 Changed 6 years ago by Michael Wedel
Refs #10996. Cleaning up PoldiSpectrumDomainFunction
Changeset: 2a63581662114f61d0cf7aed39e30d4e1cdeb6e7
comment:17 Changed 6 years ago by Michael Wedel
Refs #10996. Adding unit tests for LocalJacobian
Changeset: 5a1ac75297384f497cc2c29841e19b4299e23758
comment:18 Changed 6 years ago by Michael Wedel
Refs #10996. Providing fallback peak profile
When PoldiPeakCollection has no profile function set, the one provided to PoldiFitPeaks2D should be used.
Changeset: c44d496b258d714301e97b4379888e2ff3786720
comment:19 Changed 6 years ago by Michael Wedel
Refs #10996. Adjusting system test for POLDIFitPeaks2D
Since intensity calculation changed slightly, it's not so useful to compare those. Instead a fit is performed and it's checked whether the fitted parameters are within acceptable ranges.
Changeset: a3e51c4ffabb983ffa418aa9dab791112acb8e8e
comment:20 Changed 6 years ago by Michael Wedel
Refs #10996. Actually using fallback profile
Changeset: 14eaf02a3098262fff5c0431b654bee1de489657
comment:21 Changed 6 years ago by Michael Wedel
Refs #10996. Removing unused code from PoldiTimeTransformer
Changeset: 1177aef436748274e707bc7e0c97fb1b19dae322
comment:22 Changed 6 years ago by Michael Wedel
- Status changed from inprogress to verify
- Resolution set to fixed
This is being verified as pull request #270.
comment:23 Changed 6 years ago by Michael Wedel
Jenkins retest this please
comment:24 Changed 6 years ago by Michael Wedel
Jenkins retest this please
comment:25 Changed 6 years ago by Michael Wedel
Testing information Make sure the modified system test for PoldiFitPeaks2D passes. For manual testing you can try the usage examples of PoldiFitPeaks2D to see if they give reasonable results. To test the new feature, you could add the PeakProfilFunction-parameter to the algorithm call, for example with Lorentzian (assuming you already executed the usage example):
` PoldiFitPeaks2D(InputWorkspace=truncated_6904,
PoldiPeakWorkspace="peaks_refined_6904", PeakProfileFunction="Lorentzian", OutputWorkspace="simulated_6904", RefinedPoldiPeakWorkspace="peaks_fit_2d_6904", Calculated1DSpectrum="simulated_1d_6904", LinearBackgroundParameter=0.01)
`
In this case the spectrum has much higher values and in the 1D spectrum the Lorentzian peaks are nicely visible.
comment:26 Changed 6 years ago by Michael Wedel
This is being verified as pull request #16.
comment:27 Changed 6 years ago by Michael Wedel
Refs #10996. Correcting unit test
The unit test for the derivative was disabled, re-enabled it.
Changeset: dede5b36681e67aa1f6257919bf62b4c574c80ac
comment:28 Changed 6 years ago by Federico M Pouzols
- Status changed from verify to verifying
- Tester set to Federico M Pouzols
comment:29 Changed 6 years ago by Federico M Pouzols
All looks perfect to me and the examples seem to work well. But I found an issue in one POLDI system test. POLDIFitPeaks2DTest fails with this error:
`
File "<string>", line 1, in <module> File "/home/fedemp/mantid-repos/systemtests/StressTestFramework/stresstesting.py", line 194, in execute
self.runTest()
File "/home/fedemp/mantid-repos/systemtests/SystemTests/AnalysisTests/POLDIFitPeaks2DTest.py", line 15, in runTest
self.analyseResults(dataFiles)
File "/home/fedemp/mantid-repos/systemtests/SystemTests/AnalysisTests/POLDIFitPeaks2DTest.py", line 56, in analyseResults
self.assertTrue(np.all(absDiff < 7e-4))
File "/usr/lib/python2.7/unittest/case.py", line 422, in assertTrue
raise self.failureException(msg)
AssertionError: False is not true `
Can you check if you also get this issue. It looks like a small numerical difference could have broken the test, but it could be something local here. For me, there are values in absDiff that are definitely bigger than the 7e-4, and one of them is as big as 0.42630710526.
Otherwise I think this is ready to be merged.
comment:30 Changed 6 years ago by Michael Wedel
This is still the old system test. I modified it, because the intensity calculation is a bit different now and it simply does not give the same intensities anymore. For the new system test there is also a pull request, it's this one: https://github.com/mantidproject/systemtests/pull/16. Sorry for not making it more prominent in the testing comment.
comment:31 Changed 6 years ago by Federico M Pouzols
Ops, sorry I had missed this parallel branch. When I use this modified test with PR #270(https://github.com/mantidproject/mantid/pull/270) I get what appears to be an approximation issue:
`
File "<string>", line 1, in <module> File "/home/fedemp/mantid-repos/systemtests/StressTestFramework/stresstesting.py", line 194, in execute
self.runTest()
File "/home/fedemp/mantid-repos/systemtests/SystemTests/AnalysisTests/POLDIFitPeaks2DTest.py", line 15, in runTest
self.analyseResults(dataFiles)
File "/home/fedemp/mantid-repos/systemtests/SystemTests/AnalysisTests/POLDIFitPeaks2DTest.py", line 67, in analyseResults
self.assertLessThan(np.fabs(value - reference), error)
File "/home/fedemp/mantid-repos/systemtests/StressTestFramework/stresstesting.py", line 402, in assertLessThan
raise Exception(msg)
Exception: Expected 7.1583e-05 < 6.9e-05 `
When I enable the debug print I see this: ` 0 d 0.153846153851 1.108644 0 FWHM (rel.) 0.256706666667 0.002475747 0 Intensity 0.42762694823 3286.152 1 d 0.333333333331 1.637539 1 FWHM (rel.) 1.03743478261 0.002516417 `
It looks perfectly acceptable to me to have an approximation error in the 6th digit when using single precision. I'm not sure what Python (in different versions) may be doing here, especially when it casts the values as strings to floats. It these number above look good to you maybe the solution could be to increase a bit the error threshold and/or use double precision.
comment:32 Changed 6 years ago by Michael Wedel
Refs #10996. Removing FWHM from test until #10515 is solved.
Changeset: 4ed667abbc93314d0b87737c49e22e3673e7c388
comment:33 Changed 6 years ago by Michael Wedel
Jenkins retest this please
comment:34 Changed 6 years ago by Michael Wedel
Jenkins retest this please
comment:35 Changed 6 years ago by Michael Wedel
Refs #10996. Updating PoldiAnalyseResiduals doctest
Because of different intensity calculation, the residuals are of course also slightly different, I forgot about that before.
Changeset: e079688d63d2b47ce0fa5527c9c70f86976fde5a
comment:36 Changed 6 years ago by Michael Wedel
Refs #10996. Small fix to FunctionParameterDecorator
Forwarding of active parameters was not implemented.
Changeset: c46c8a63fc16d15ca9b95a36d85b218c8d8a2ccd
comment:37 Changed 6 years ago by Michael Wedel
Re #10996. PoldiSpectrumDomainFunction uses FunctionParameterDecorator
Changeset: 714929fbd84a9cfddf4ba4a31af74d9d9caf0370
comment:38 Changed 6 years ago by Michael Wedel
Refs #10996. Adjusting unit test.
Changeset: 9c5e78c120fc7e3bc8c2e0fceacd8c360bbadc7e
comment:39 Changed 6 years ago by Michael Wedel
Refs #10996. Adjusting unit test for PeakParameterFunction
Changeset: 5152fbccf20353beaf343d6a49c539b469a80a90
comment:40 Changed 6 years ago by Michael Wedel
Refs #10996. Fixing system test for PoldiFitPeaks2D
Changeset: 5924fced90f869d942bdce23585a7efe777121e3
comment:41 Changed 6 years ago by Michael Wedel
PoldiSpectrumDomainFunction has been updated to use changes from ticket #11102(http://trac.mantidproject.org/mantid/ticket/11102), so that wrapping of the peak function is simpler. This is just an implementation detail, it does not change the testing information.
Additionally, the system test has been updated, so the systemtests pull request is obsolete.
comment:42 Changed 6 years ago by Michael Wedel
Jenkins retest this please
comment:43 Changed 6 years ago by Federico Montesino Pouzols
All is good after the system tests transition to the main repo.
comment:44 Changed 6 years ago by Federico Montesino Pouzols
- Status changed from verifying to closed
Merge pull request #270 from mantidproject/feature/10996_refactor_poldi_spectrum_domain_function
Refactor PoldiSpectrumDomainFunction to use arbitrary peak profile functions
Full changeset: a70d395ad6a79d86a02b25a4f385a14efbc3ab7f
comment:45 Changed 6 years ago by Michael Wedel
System test is in main repo now, so I'm closing this pull request.
comment:46 Changed 5 years ago by Stuart Campbell
This ticket has been transferred to github issue 11835
Refs #10996. Alternative approach for calculation of POLDI 2D-spectrum
Changeset: 06a6c1b655727389fd3d7e01b4a409966fc5f2d5