Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor PoldiSpectrumDomainFunction to use arbitrary peak profile functions #270

Merged

Conversation

MichaelWedel
Copy link
Contributor

This fixes #10996.

Michael Wedel added 20 commits January 30, 2015 08:39
This is no longer required and is removed.
When PoldiPeakCollection has no profile function set, the one provided to PoldiFitPeaks2D should be used.
@MichaelWedel MichaelWedel added this to the Release 3.4 milestone Feb 20, 2015
@MichaelWedel MichaelWedel added Diffraction Issues and pull requests related to diffraction In Progress Powder Issues and pull requests related to powder diffraction labels Feb 20, 2015
@MichaelWedel
Copy link
Contributor Author

Jenkins retest this please

1 similar comment
@MichaelWedel
Copy link
Contributor Author

Jenkins retest this please

@MichaelWedel
Copy link
Contributor Author

Testing information
Make sure the modified system test for PoldiFitPeaks2D passes (pull request is here). For manual testing you can try the usage examples of PoldiFitPeaks2D to see if they give reasonable results (output should be similar to what is shown in the documentation - don't worry about the 0-spectra, these are because of excluded wires). 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.

The unit test for the derivative was disabled, re-enabled it.
@FedeMPouzols FedeMPouzols self-assigned this Feb 25, 2015
@FedeMPouzols
Copy link
Contributor

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.

@MichaelWedel
Copy link
Contributor Author

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: mantidproject/systemtests#16. Sorry for not making it more prominent in the testing comment.

@MichaelWedel
Copy link
Contributor Author

Jenkins retest this please

1 similar comment
@MichaelWedel
Copy link
Contributor Author

Jenkins retest this please

@MichaelWedel
Copy link
Contributor Author

PoldiSpectrumDomainFunction has been updated to use changes from 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.

@MichaelWedel
Copy link
Contributor Author

Jenkins retest this please

@FedeMPouzols
Copy link
Contributor

All is good after the system tests transition to the main repo.

FedeMPouzols added a commit that referenced this pull request Mar 6, 2015
…di_spectrum_domain_function

Refactor PoldiSpectrumDomainFunction to use arbitrary peak profile functions
@FedeMPouzols FedeMPouzols merged commit a70d395 into master Mar 6, 2015
@FedeMPouzols FedeMPouzols deleted the feature/10996_refactor_poldi_spectrum_domain_function branch March 6, 2015 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Diffraction Issues and pull requests related to diffraction Powder Issues and pull requests related to powder diffraction
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants