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

Fit algorithm with MultiDomainFunction stores some workspaces in ADS even if StoreInADS=False #38476

Open
RichardWaiteSTFC opened this issue Dec 5, 2024 · 0 comments
Labels
Bug Issues and pull requests that are regressions or would be considered a bug by users (e.g. crashing) ISIS Team: Core Issue and pull requests managed by the Core subteam at ISIS
Milestone

Comments

@RichardWaiteSTFC
Copy link
Contributor

RichardWaiteSTFC commented Dec 5, 2024

Describe the bug
Fit algorithm with MultiDomainFunction stores workspaces in ADS even if StoreInADS=False

To Reproduce
(1) Run this script

from mantid.simpleapi import *
from mantid.api import MultiDomainFunction, FunctionFactory
from mantid.fitfunctions import FunctionWrapper
                          

comp_func = CompositeFunction(Gaussian(Height=1, Sigma=0.3, PeakCentre=3), FunctionWrapper("FlatBackground"), NumDeriv=True)
ws = CreateSampleWorkspace(OutputWorkspace='ws', Function='User Defined', UserDefinedFunction=str(comp_func), XUnit='DeltaE', 
                           XMax=7, BinWidth=0.1, NumBanks=1, BankPixelWidth=2)

func = MultiDomainFunction()        
md_fit_kwargs = {}
for ispec in range(ws.getNumberHistograms()):
    func.add(comp_func.function)
    func.setDomainIndex(ispec,ispec)
    key_suffix = f"_{ispec}" if ispec > 0 else ""
    md_fit_kwargs["InputWorkspace" + key_suffix] = ws.name()
    md_fit_kwargs["StartX" + key_suffix] = 0.1
    md_fit_kwargs["EndX" + key_suffix] = 6.8
    md_fit_kwargs["WorkspaceIndex" + key_suffix] = ispec


out = Fit(Function=str(func), Output='ws', StoreInADS=False, CreateOutput=True, OutputCompositeMembers=True,
          **md_fit_kwargs)

(2) For StoreInADS=False get this
image

(3) For StoreInADS=True get this
image

Expected behavior
Would not have workspaces ws_Wortkspace_0 etc. in ADS when StoreInADS=False

@RichardWaiteSTFC RichardWaiteSTFC added Bug Issues and pull requests that are regressions or would be considered a bug by users (e.g. crashing) ISIS Team: Core Issue and pull requests managed by the Core subteam at ISIS labels Dec 5, 2024
@RichardWaiteSTFC RichardWaiteSTFC added this to the Release 6.12 milestone Dec 5, 2024
RichardWaiteSTFC added a commit that referenced this issue Dec 11, 2024
need to be in ADS due to bug in issue #38476
peterfpeterson pushed a commit to peterfpeterson/mantid that referenced this issue Jan 17, 2025
This is a squashed version of mantidproject#38515

Use MultiDomainFunction to tie peak params accross pixels

Fit only pixels I/sig > min using skew detemrined background

Have defined min I/sig in input validator as 0.5 (previously 0)

Skip peak early if not in x-range of data

Delete fit workspaces from ADS

need to be in ADS due to bug in issue mantidproject#38476

Fix bug in logic for peaks on edge

Fix typo in fit result status

Add check for any True in final fit peak mask

Update unit test I/sig values and number edge pixels

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)

Fix bug in intensity estimation fixing poisson cost func test

Test was failing as initial guess was far from data - this was because hadn't mutliplied counts by bin-width when integrating

Add colorbar to plot

Add option to specify minimum number of pixels in peak (and test)

Add test for fixing peak params

Fix test for d-spacing tolerance - was not testing actual behaviour

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.

Add constraint on FWHM to stop peak fitting background or noise

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

Reduce bin-width in unit test

In limit where FWHM comparabel to bin-width (which is about to be one of post-fit checks impose)

Add post fit check on peak FWHM

And loosen constrints on FWHM by factor 2

Fix hessian error strategy unit test

Fix unit test I/sig values and fix doc test

Have increased as capturing more of the peak

Remove duplicate code for freeing/removing ties

Add release note

Update doc test value - not sure why this changed since last time...

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Issues and pull requests that are regressions or would be considered a bug by users (e.g. crashing) ISIS Team: Core Issue and pull requests managed by the Core subteam at ISIS
Projects
Status: No status
Development

No branches or pull requests

1 participant