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 switched nomenclature for length and width #533

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

caitwolf
Copy link
Contributor

Found a location in resolution.py where the Slit_Resolution function was called with width and length switched. It seemed that the inputs were switched but so were the operations to length and width in the function so it was performing the correct calculations. However, @pbeaucage and @butlerpd should take a look at this before merging considering the recent changes to the slit smearing functions.

@caitwolf caitwolf marked this pull request as ready for review October 30, 2022 16:16
Copy link
Contributor

@pbeaucage pbeaucage left a comment

Choose a reason for hiding this comment

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

looks good

@wpotrzebowski
Copy link

It seems like this is something that should be covered by unit test. Would it be difficult to create one?

@pbeaucage
Copy link
Contributor

It's already well covered by unit tests but in this case the values were flipped twice, resulting in tests improperly passing. In some sense, then, this is just a code clarity issue, not a code correctness issue.

@caitwolf
Copy link
Contributor Author

Looking further into the unit tests in resolution.py, it looks like many of them are turned off. @pkienzle do you know why these tests were switched off?

@pkienzle
Copy link
Contributor

Variable name "l" is heavily contraindicated. Too many fonts confuse "1", "l" and "I".

Could you change the loops to use i, (qi, wi, li) or maybe i, (q_i, width_i, length_i)?

And there's a couple of more instances, such as function parameters and direct assignments (23 uses of l and 28 uses of w in total),

[Yes, I should have noted this when "h" was changed to "l".]

@caitwolf
Copy link
Contributor Author

I updated the single letter variable names for length and width where I could find them, but let me know if I missed any others hiding somewhere.

@caitwolf
Copy link
Contributor Author

Looking further into the unit tests in resolution.py, it looks like many of them are turned off. @pkienzle do you know why these tests were switched off?

This is an example of the unit tests that seem to be turned off currently:

@unittest.skip("not yet supported")
def test_slit_long(self):
"""
Slit smearing with length 0.005
"""
resolution = Slit1D(self.x, q_width=0, q_length=0.005, q_calc=self.x)
theory = self.Iq(resolution.q_calc)
output = resolution.apply(theory)
answer = [
9.0618, 8.6402, 8.1187, 7.1392, 6.1528,
5.5555, 4.5584, 3.5606, 2.5623, 2.0000,
]
np.testing.assert_allclose(output, answer, atol=1e-4)

@caitwolf caitwolf requested a review from pkienzle October 31, 2022 20:37
@pkienzle
Copy link
Contributor

pkienzle commented Nov 1, 2022

It looks like I turned them off when I rewrote the code but didn't reset them when the rewrite was approved. Instead I have a limited validation test in sasmodels.resolution.IgorComparisonTest.test_ellipsoid. So now we are in a state where the old tests are incorrect and the new tests are incomplete.

The question is where do we get the target values for the test. For simple verification we can use the current values from the code as the previous test did. Then we know that the code runs and is still producing the same values. This doesn't tell us that the code is correct. For that we need to validate against an analytic form, or against a high precision numerical integration if no simple analytic solutions are available.

Here's a validation for a sphere of radius R=100 with Δρ=1 for width=0.01 at q=0.015 (i.e., qx ∈ [0.01, 0.02], qy ∈ [–∞, ∞]) using wolfram alpha:

integrate 4/3*pi*100^3*9*((sin(sqrt(x^2+y^2)*100)-sqrt(x^2+y^2)*100*cos(sqrt(x^2+y^2)*100))/(sqrt(x^2+y^2)*100)^3)^2 for x in [0.01,0.02] y in [-inf,inf]

giving a value of 956.701 (there's probably some unit correction scale factor that I'm missing).

I'm doing something similar in test_ellipsoid using scipy numerical integration, but only for one pair of (width, length). The numerical integration can be expensive but it doesn't have to be part of the test suite. Instead we can move the validation code to sasmodels/explore/resolution_validation.py and have that produce values for the unit tests. In particular, it can produce the new target values for the old unit tests.

@caitwolf caitwolf marked this pull request as draft November 8, 2022 14:52
Copy link
Contributor

@pkienzle pkienzle left a comment

Choose a reason for hiding this comment

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

Maybe we should be using dq_perp and dq_par instead of length and width everywhere? Much less confusing than length and width.

@@ -157,7 +157,7 @@ def __init__(self, q, q_length=None, q_width=None, q_calc=None):

# Build weight matrix from calculated q values
self.weight_matrix = (
slit_resolution(self.q_calc, self.q, q_length, q_width)
slit_resolution(self.q_calc, self.q, q_width, q_length)
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than reversing the order of the parameters here, please reverse the order of the parameters in the slit_resolution method. That way Slit1D (the external API) and slit_resolution will have the same order and future swapping errors a little less likely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched back to q_length, q_width here and updated slit_resolution

@@ -339,7 +339,6 @@ def slit_resolution(q_calc, q, width, length, n_length=30):

Copy link
Contributor

Choose a reason for hiding this comment

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

On line 85:

to $q_j \in \left[q, \sqrt{q^2 + \Delta q_\perp}\right]$, so

the $q_\perp$ term has to be squared for the units to work. Which is to say that we should reread and recheck the equations in the doc string.

Copy link
Contributor Author

@caitwolf caitwolf Jan 17, 2024

Choose a reason for hiding this comment

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

I've corrected the $q_\perp$ term in the doc string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this time I have not done a thorough read of the doc string to check for additional typos in the equations.

_int_w = lambda w, qi: eval_form(sqrt(qi**2 + w**2), form, pars)
_int_l = lambda l, qi: eval_form(abs(qi+l), form, pars)
_int_w = lambda wi, qi: eval_form(sqrt(qi**2 + wi**2), form, pars)
_int_l = lambda li, qi: eval_form(abs(qi+li), form, pars)
Copy link
Contributor

Choose a reason for hiding this comment

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

The $w_i$, $l_l$ labels here are inconsistent with length going to $q_\perp$ (adding in quadrature) and width going to $q_\parallel$ (adding linearly).

I think we should change the romberg_slit_1d signature to use length, width rather than width, length for consistency, and reverse the use of length and width within this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the romberg_slit_1d function to use length, width rather than width, length and reversed the use of length and width within the function. However, this led me to a similar change for slit_extend_q that was still using width, length. It appeared as though slit_extend_q was called in different ways so I've tried to correct this in the appropriate places. The tests that are still implemented are passing, but this will need careful review.

@butlerpd
Copy link
Member

butlerpd commented Aug 6, 2023

Hummm... interesting idea - dqperp and par would be correct and match how we do pinhole (I think). My main concern is that the community seems to have settled on l and w as defined in the NXcanSAS standard ... so it may increase rather than decrease confusion by "professional" sas practioners?

…s-per-decade-is-provided-or-data-is-a-single-point
@butlerpd
Copy link
Member

Since it works (though for the wrong reason?) probably not a high priority for 6.0 but nice to have fixed properly.

@caitwolf
Copy link
Contributor Author

@pkienzle @butlerpd I've gone through and made additional changes based on @pkienzle's last review. To summarize, this fixed the slit smearing related functions to use arguments of length, width rather than width, length to minimize swapped parameters in the future. However, this requires another careful review before we can merge, especially if there are other parts of the code I've missed that are using these functions still assuming the swapped ordering of arguments.

At first this PR was primarily a code clarity issue that was limited to a single method however this has now been more broadly applied to this switched nomenclature found in multiple places within resoluiton.py.

I also propose moving the update of the test functions discussed above to a separate issue to keep this PR focused on the switched nomenclature of the length and width parameters.

@caitwolf caitwolf requested a review from pkienzle January 17, 2024 19:57
@caitwolf caitwolf marked this pull request as ready for review January 17, 2024 19:58
…when-points-per-decade-is-provided-or-data-is-a-single-point

Fixed inverse q-spacing in the geometric extrapolation function
@caitwolf caitwolf marked this pull request as draft January 22, 2024 21:34
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.

5 participants