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

Fixed inverse q-spacing in the geometric extrapolation function #571

Conversation

caitwolf
Copy link
Contributor

This PR fixes the geometric extrapolation bug outlined in #568. The log spacing for the q extrapolation was calculated as the inverse term depending on whether or not points_per_decade was specified when calling the function, resulting in two different outputs.

caitwolf and others added 2 commits June 27, 2023 13:36
…s-per-decade-is-provided-or-data-is-a-single-point
…og_delta_q is calculated when points_per_decade is specified
@caitwolf caitwolf marked this pull request as draft June 27, 2023 17:42
@pkienzle
Copy link
Contributor

Testing with:

geometric_extrapolation(np.array([1e-4]), 1e-5, 1e-3, points_per_decade=4)

gives an extra point in the preceding decade:

[1.00000000e-05, 1.58489319e-05, 2.51188643e-05, 3.98107171e-05,
        6.30957344e-05, 1.00000000e-04, 1.77827941e-04, 3.16227766e-04,
        5.62341325e-04, 1.00000000e-03]

because log_delta_q * (log(q[0])-log(q_min)) evaluates to 4.000000000000001 when computing n_low.

We may find that we have different results on different computers because of small differences in floating point processing, but this will likely be less than the effect of using single rather than double precision if a GPU is available so I'm inclined to ignore it.

@pkienzle
Copy link
Contributor

I'm not sure why this is marked as draft, but it seems ready to merge.

@butlerpd
Copy link
Member

@caitwolf, Paul thinks this is ready to move from draft. Did you have some issues you were holding this back for? In particular do we want this to be in the sasmodels release that will build SasView 6.0?

@butlerpd
Copy link
Member

@caitwolf , @dehoni @rmdalgliesh @gnsmith @wimbouwman - Should this be part of 6.0?

…s-per-decade-is-provided-or-data-is-a-single-point
@caitwolf
Copy link
Contributor Author

@caitwolf , @dehoni @rmdalgliesh @gnsmith @wimbouwman - Should this be part of 6.0?

@butlerpd was this question for PR #536 after our discussion on the call?

@caitwolf caitwolf self-assigned this Nov 21, 2023
@caitwolf caitwolf marked this pull request as ready for review November 21, 2023 15:46
@caitwolf caitwolf marked this pull request as draft November 21, 2023 17:20
@caitwolf
Copy link
Contributor Author

Converting this back to a draft, many of the checks for slit smearing a model are broken after the change.

@caitwolf caitwolf requested a review from pkienzle November 21, 2023 17:20
@butlerpd
Copy link
Member

Oops .. I think so. Sorry and thanks for the catch

@caitwolf
Copy link
Contributor Author

Converting this back to a draft, many of the checks for slit smearing a model are broken after the change.

@pkienzle it looks like the opencl tests are failing specifically. Would you be able to take a look?

@caitwolf
Copy link
Contributor Author

Converting this back to a draft, many of the checks for slit smearing a model are broken after the change.

@pkienzle it looks like the opencl tests are failing specifically. Would you be able to take a look?

@pkienzle I'm circling back to this pull request during contributor camp; would you be able to help me out with the opencl tests that are failing with this change?

@caitwolf caitwolf marked this pull request as ready for review January 16, 2024 16:57
@caitwolf
Copy link
Contributor Author

Determined that failing opencl tests is external of this PR and is more broadly affecting the master branch and all PR's.

@pkienzle this fix will affect test_simple_interface in direct_model.py. As discussed, can you update the target values?

@caitwolf caitwolf changed the base branch from master to use-dQ-Data-slit-length-and-width-switched January 19, 2024 15:51
@caitwolf caitwolf merged commit 2fc80e0 into use-dQ-Data-slit-length-and-width-switched Jan 19, 2024
0 of 24 checks passed
@caitwolf caitwolf deleted the 568-fix-geometric-extrapolation-when-points-per-decade-is-provided-or-data-is-a-single-point branch January 19, 2024 15:53
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.

Fix geometric extrapolation when points-per-decade is provided or data is a single point
4 participants