Skip to content

fast fitting: simplify loop logic #1131

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

Closed
wants to merge 2 commits into from
Closed

Conversation

woutdenolf
Copy link
Collaborator

@woutdenolf woutdenolf commented Jun 21, 2025

We have been using FastXRFLinearFit with a patched OutputBuffer for a while in ewoks to have full control over the output but we had to disable refitting since it's a one-way street. In addition this code was insanely complex. So I decided to bite the bullet and use ClassMcaTheory with ConcentrationsTool directly.

I'm in the process of dissecting the FastXRFLinearFit code and several merge requests will follow.

This MR tackles only the loop itself. In a follow-up I will tackle the bad pixel stuff. There is another MR on the xmin/xmax limits.

def original_while_loop(nFreeBkg, nFree):
    iIter = 1
    nIter = 2 * (nFree - nFreeBkg) + iIter
    negativePresent = True

    nfits = 0
    print(nIter)

    while negativePresent:
        if iIter > nIter:
            negativePresent = False
            print("max")
            continue
        nfits += 1  # refit
        iIter += 1

    print(nfits)


def new_for_loop_with_break(nFreeBkg, nFree):
    nPeakAreas = nFree - nFreeBkg
    nrefitMax = 2 * nPeakAreas + 1

    nfits = 0
    print(nrefitMax)

    for refitIter in range(1, nrefitMax + 2):
        if refitIter > nrefitMax:
            print("max")
            break
        nfits += 1  # refit

    print(nfits)


if __name__ == "__main__":
    original_while_loop(0, 5)
    print()
    new_for_loop_with_break(0, 5)
    print()
    original_while_loop(5, 5)
    print()
    new_for_loop_with_break(5, 5)

@woutdenolf woutdenolf requested a review from vasole June 21, 2025 18:03
@woutdenolf
Copy link
Collaborator Author

What is the reason for the maximum number of refits to be 2 * nPeakAreas + 1?

@vasole
Copy link
Member

vasole commented Jun 23, 2025

If you have repeated the fit for all the elements twice, you surely are in an endless loop. Therefore a limit based on the number of fitted peak areas.

@vasole
Copy link
Member

vasole commented Jun 23, 2025

We have been using FastXRFLinearFit with a patched OutputBuffer for a while in ewoks to have full control over the output but we had to disable refitting since it's a one-way street. In addition this code was insanely complex. So I decided to bite the bullet and use ClassMcaTheory with ConcentrationsTool directly.

The original/legacy implementation without OutputBuffer was simpler and on those lines.

@vasole
Copy link
Member

vasole commented Jun 23, 2025

The change on the limits is to be reverted. McaAdvancedFitBatch and FastXRFLinearFit follow the same philosophy: if no limits are passed, the limits in the fit configuration are used.

use_limits was implemented on user request to avoid having irreproducible interactive sessions because of not selecting exactly the same zoom region.

@woutdenolf woutdenolf closed this Jun 24, 2025
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.

2 participants