search

SasView / sasmodels

Package for calculation of small angle scattering models using OpenCL.
BSD 3-Clause "New" or "Revised" License
16 stars 28 forks source link

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

Closed caitwolf closed 10 months ago

caitwolf commented 1 year ago

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.

pkienzle commented 1 year ago

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 commented 1 year ago

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

butlerpd commented 1 year ago

@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 commented 1 year ago

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

caitwolf commented 1 year ago

@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 commented 12 months ago

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

butlerpd commented 12 months ago

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

caitwolf commented 11 months ago

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 commented 10 months ago

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 commented 10 months ago

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?