Ticket #10996 (closed: fixed)

Opened 6 years ago

Last modified 5 years ago

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:1 Changed 6 years ago by Michael Wedel

Refs #10996. Alternative approach for calculation of POLDI 2D-spectrum

Changeset: 06a6c1b655727389fd3d7e01b4a409966fc5f2d5

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:6 Changed 6 years ago by Nick Draper

  • Status changed from new to assigned

comment:7 Changed 6 years ago by Michael Wedel

  • Blocked By 11104 added

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

Note: See TracTickets for help on using tickets.