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.

[pbeaucage]

looks good

[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:

sasmodels/sasmodels/resolution.py

Lines 659 to 671 in fd03bda

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

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

[pkienzle]

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

[pkienzle]

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.

[caitwolf]

Switched back to q_length, q_width here and updated slit_resolution

[pkienzle]

On line 85:

sasmodels/sasmodels/resolution.py

Line 285 in 5bdfdce

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.

[caitwolf]

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

[caitwolf]

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

[pkienzle]

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.

[caitwolf]

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]

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.