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

Use MultiDomainFunction in IntegratePeaks1DProfile #38515

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

RichardWaiteSTFC
Copy link
Contributor

Description of work

Summary of work

Fixes #xxxx.

Further detail of work

To test:


Reviewer

Please comment on the points listed below (full description).
Your comments will be used as part of the gatekeeper process, so please comment clearly on what you have checked during your review. If changes are made to the PR during the review process then your final comment will be the most important for gatekeepers. In this comment you should make it clear why any earlier review is still valid, or confirm that all requested changes have been addressed.

Code Review

  • Is the code of an acceptable quality?
  • Does the code conform to the coding standards?
  • Are the unit tests small and test the class in isolation?
  • If there is GUI work does it follow the GUI standards?
  • If there are changes in the release notes then do they describe the changes appropriately?
  • Do the release notes conform to the release notes guide?

Functional Tests

  • Do changes function as described? Add comments below that describe the tests performed?
  • Do the changes handle unexpected situations, e.g. bad input?
  • Has the relevant (user and developer) documentation been added/updated?

Does everything look good? Mark the review as Approve. A member of @mantidproject/gatekeepers will take care of it.

Gatekeeper

If you need to request changes to a PR then please add a comment and set the review status to "Request changes". This will stop the PR from showing up in the list for other gatekeepers.

Have defined min I/sig in input validator as 0.5 (previously 0)
need to be in ADS due to bug in issue #38476
Number edge pixels changed because previously any pixels on the detector edge attempted to be fitted would be classed as "on  edge".
Here we fit a much larger number of pixels, so the condition is updated to be only the successful fits (rather than all attempted)
Test was failing as initial guess was far from data - this was because hadn't mutliplied counts by bin-width when integrating
To do this offset the simulated peaks in one of the two pixels containing the peak - when the center is free to vary (unconstrained by the DIFC ratio or user imposed limits) in the final/second fit, the same solution will be reached.
Hence the I/sig values unchanged, with one exception: I have updated the I/sig test value for Poisson fit - the final fit is still good, just slightly different.
Same constraint as used in main.
Have needed to adjust I/sig of Gaussian unit test as bin-widths are quite large relative to FWHM. If you set fwhm_min -> 0.5 fwhm_min then get initial I/sig
In limit where FWHM comparabel to bin-width (which is about to be one of post-fit checks impose)
And loosen constrints on FWHM by factor 2
@RichardWaiteSTFC RichardWaiteSTFC marked this pull request as draft December 16, 2024 14:38
@RichardWaiteSTFC RichardWaiteSTFC force-pushed the 38439_use_MD_fitting_IntegratePeaks1DProfile branch from a1a6322 to 7bb3bfc Compare December 18, 2024 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant