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

Fixes binning bug in implementation of sesans data #2331

Merged
merged 2 commits into from Oct 30, 2022

Conversation

caitwolf
Copy link
Contributor

@caitwolf caitwolf commented Oct 29, 2022

This fixes a binning bug for sesans data.

Sesans is in real space and the Hankel transform from q space to real space (spin echo length) was implemented as a resolution function. For other resolution functions, the qmin and qmax range is compared to data.x which are all in q space. This is then passed to the calculator to return the unsmeared Iq. However, for sesans data, qmin, qmax, and data.x are all in real space while the calculator is expecting q space data to return the unsmeared Iq (which will then get passed to the sesans "smearer" which uses the hankel transform to convert Iq to real space).

Due to the number of points in q_calc (in q space) being much greater than the number of points in q (spin echo length in real space) in the sesans transform (resolution function), this resulted in a small portion of the data being improperly calculated at the incorrect q values (spin echo length being passed as q to the calculator returning Iq) with a error on the order of 1e-10.

The q, Iq data being passed to the Hankel transform before and after the fix for a sphere form factor with scale = 0.005, background = 0, sld = 1, sld_solvent = 6, and radius = 10000:
image

After the Hankel transform for both q, Iq datasets and compared to the analytical model for the same system:
image

Error between the datasets in SESANS space:
image

@caitwolf caitwolf marked this pull request as ready for review October 29, 2022 13:20
Copy link
Contributor

@lucas-wilkins lucas-wilkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it should fix it. I take it that it looks right in the test script you have.

Also, you had some numbers quantifying the size of the error it produced. Can you put them here (in the PR description)?

Copy link
Member

@butlerpd butlerpd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clearly I'm not going to see any difference from my GUI tests, but running the installer doesn't show any obvious new issues. Code seems appropriate and minimalistic changes. Suggest we merge

Copy link

@wpotrzebowski wpotrzebowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Played with it on Mac OSX and it seems to work fine both on sesans and regular data sets

@wpotrzebowski wpotrzebowski merged commit 457eb17 into main Oct 30, 2022
@wpotrzebowski wpotrzebowski deleted the sesans-debug-binning branch October 30, 2022 15:14
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.

None yet

4 participants