fixed switched nomenclature for length and width

[caitwolf]

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.

[wpotrzebowski]

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

[pbeaucage]

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]

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]

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]

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]

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: https://github.com/SasView/sasmodels/blob/fd03bdaca12b3384d3a02ca8d0635fb4a8da66ef/sasmodels/resolution.py#L659-L671

[pkienzle]

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.

[butlerpd]

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?

[butlerpd]

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

[caitwolf]

@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.