q_to_tth & tth_to_q #178

yucongalicechen · 2024-11-15T13:42:31Z

closes #174
@sbillinge ready for review!

codecov · 2024-11-15T13:44:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.62%. Comparing base (9789eac) to head (d481253).
Report is 17 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #178      +/-   ##
==========================================
+ Coverage   99.17%   99.62%   +0.44%     
==========================================
  Files           7        7              
  Lines         242      265      +23     
==========================================
+ Hits          240      264      +24     
+ Misses          2        1       -1

Files with missing lines	Coverage Δ
...ils/scattering_objects/test_diffraction_objects.py	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

src/diffpy/utils/scattering_objects/diffraction_objects.py

sbillinge

pls see comments inline. Per our earlier convo, I think we should be working on these in the tests first. Let's converge the tests first, then write the code. What is the behavior that we want to handle? I know it is something like tth exceeding 180 deg, but when would this come about in practice? We only need to handle it if it is a user-facing issue, so what would a user do where this would be encountered? then how do we want the code to behave to alert the user?

sbillinge · 2024-11-15T20:44:17Z

src/diffpy/utils/scattering_objects/diffraction_objects.py

        """
        q = self.on_q[0]
        q = np.asarray(q)
        wavelength = float(self.wavelength)
        pre_factor = wavelength / (4 * np.pi)
+        if np.any(np.abs(q * pre_factor) > 1):
+            raise ValueError(
+                "Invalid input for arcsin: some values exceed the range [-1, 1]. Check wavelength or q values."


I think the users will not have a clear idea what has gone wrong and how to fix it from this. I think we should not raise a ValueError, but just let the math fail, but overload the error message with advice about how to fix it.

sbillinge · 2024-11-16T02:00:47Z

src/diffpy/utils/scattering_objects/diffraction_objects.py

            of ``wavelength``
        """
        two_theta = np.asarray(np.deg2rad(self.on_tth[0]))
+        if np.any(two_theta > np.pi):


as above, I think this isn't doing quite what we want.

yucongalicechen · 2024-11-20T17:03:21Z

src/diffpy/utils/scattering_objects/diffraction_objects.py

        """
        q = self.on_q[0]
        q = np.asarray(q)
        wavelength = float(self.wavelength)
        pre_factor = wavelength / (4 * np.pi)
+        if np.any(np.abs(q * pre_factor) > 1):
+            raise ValueError("Please check if you entered an incorrect wavelength or q value.")


I think we would want to raise a value error here because otherwise the code proceeds with tth = nan.

sbillinge

Let's first work on the test and then the code, so don't modify the code yet, just the test. The test should test the error message that is sent to users under different situations. The most likely one we discussed is that they give a wrong wavelength. They could also give no wavelength. What behavior do we want then? Less likely but possible is they give a tth array that goes above 180. What behavior do we want then?

yucongalicechen · 2024-11-21T23:21:53Z

Let's first work on the test and then the code, so don't modify the code yet, just the test. The test should test the error message that is sent to users under different situations. The most likely one we discussed is that they give a wrong wavelength. They could also give no wavelength. What behavior do we want then? Less likely but possible is they give a tth array that goes above 180. What behavior do we want then?

Yes, sorry, I added more tests now. @sbillinge it looks like in DO tth is implemented on degree scale only, shall we make another issue to allow tth to compute in radian?

sbillinge

Please see inline

sbillinge · 2024-11-22T13:30:49Z

tests/diffpy/utils/scattering_objects/test_diffraction_objects.py

-    actual = DiffractionObject(wavelength=4 * np.pi)
-    setattr(actual, "on_q", [[0.6, 0.8, 1, 1.2], [1, 2, 3, 4]])
+params_q_to_tth_bad = [
+    # UC1: user did not specify wavelength


Do we want to not allow missing wavelengths? This makes these DOs harder to use, but also makes them less useful. There may be a middle ground where we allow it, but let the user know that much of the functionality goes away without it. We could also trigger a workflow that requests the wavelength, but still allows users to override that. This would encourage them to enter a wavelength but not insist?

Yes this sounds good. I think user can also directly set attributes so that they can use DO without a wavelength. I would suggest to prompt user inputs in the insert_scattering_quantity function instead of this one when it wants to set arrays on all tth/q/dspace, so that it can avoid calling these functions?
When people use this function directly we would want them to speicify a wavelength, so it's better if we raise an error?

For now, how about we just print a warning message if no wavelength is supplied. Something like "INFO: no wavelength has been specified. You can continue to use the DiffractionObject but some of it's powerful features will not be available. To specify a wavelength...."

sbillinge · 2024-11-22T13:32:55Z

tests/diffpy/utils/scattering_objects/test_diffraction_objects.py

+    # UC2: user specified invalid q values that result in tth > 180 degrees
+    (
+        [4 * np.pi, [0.2, 0.4, 0.6, 0.8, 1, 1.2]],
+        "Wavelength * q > 4 * pi. Please check if you entered an incorrect wavelength or q value.",


I think this behavior is good. The first sentence is a bit mathematical and cryptic. Is there a more "chemist friendly" way of saying it?

Does "The combination of wavelength and q values is too large" sound good?

How about: "the supplied q-array and wavelength will result in an impossible two-theta. Please check these values and re-instantiate the DiffractionObject"

sbillinge · 2024-11-22T13:38:59Z

tests/diffpy/utils/scattering_objects/test_diffraction_objects.py

+        [100, [0, 0.2, 0.4, 0.6, 0.8, 1]],
+        "Wavelength * q > 4 * pi. Please check if you entered an incorrect wavelength or q value.",
+    ),
+    # UC4: user specified an empty q array


It is ok to specify no q-array (you can instantiate on tth for example). Presumably this is only an error if the the DO is being created with q data. So the behavior is ok, but we may want to tweak the error message. I think a more general error is if the x and y arrays are not the same length, whether they are q, tth or d. I suggest to handle that more general case?

Finally, do we also want to specify what error is raised as well as the message?

yucongalicechen · 2024-11-25T14:55:11Z

@sbillinge ready for review

sbillinge

Please see inline

sbillinge · 2024-11-25T19:03:44Z

tests/diffpy/utils/scattering_objects/test_diffraction_objects.py

+            "INFO: no wavelength has been specified. You can continue "
+            "to use the DiffractionObject but some of its powerful features "
+            "will not be available. To specify a wavelength, you can use "
+            "DiffractionObject(wavelength=0.71)."


0.71? Or something like "wavelength=) where you replace with the actual wavelength in the units of..." Etc. Also, are these the correct instructions? This seems to be instantiating a new DO not adding a wavelength to an existing DO.

Let's also make sure to add these instructions to the documentation.

sbillinge · 2024-11-25T19:05:09Z

tests/diffpy/utils/scattering_objects/test_diffraction_objects.py

+        [
+            ValueError,
+            "The supplied q-array and wavelength will result in an impossible two-theta. "
+            "Please check these values and re-instantiate the DiffractionObject.",


Maybe add ."... With correct values"

sbillinge · 2024-11-25T19:05:34Z

tests/diffpy/utils/scattering_objects/test_diffraction_objects.py

+        [100, [0, 0.2, 0.4, 0.6, 0.8, 1], [1, 2, 3, 4, 5, 6]],
+        [
+            ValueError,
+            "The supplied q-array and wavelength will result in an impossible two-theta. "


See above. Also since we are reusing the error message do we want to minimize word by defining it in a variable once and reusing the variable in multiple tests?

Yes I will store them in diffraction_objects.py file

sbillinge · 2024-11-25T19:08:43Z

tests/diffpy/utils/scattering_objects/test_diffraction_objects.py

-        [4 * np.pi, [0, 0.2, 0.4, 0.6, 0.8, "invalid"]],
-        "Invalid value found in q array. Please ensure all values are numeric.",
+        [4 * np.pi, [0, 0.2, 0.4, 0.6, 0.8, 1], [1, 2, 3, 4, 5]],
+        [IndexError, "Please ensure q array and intensity array are the same length."],


I think RunTime wrote may be more appropriate here since I think we will raise it ourselves?

yucongalicechen · 2024-11-25T19:37:13Z

@sbillinge thanks for the comments. Since the tests are almost there I'll start implementing the functions.

sbillinge

This looks good now good to go on the code.

sbillinge · 2024-11-26T00:43:35Z

tests/diffpy/utils/scattering_objects/test_diffraction_objects.py

@@ -231,6 +231,158 @@ def test_diffraction_objects_equality(inputs1, inputs2, expected):
    assert (diffraction_object1 == diffraction_object2) == expected


+def _test_valid_diffraction_objects(actual_diffraction_object, function, expected_array):
+    """Checks the behavior of the DiffractionObject:


Test functions don't need docstring, though if it's ok to leave this now you did it.

sbillinge

looks good. this needs a news and possibly some update in the docs. You don't have to explain error messages in docs, but it might be nice to explain correct usage.

yucongalicechen · 2024-11-26T13:22:01Z

looks good. this needs a news and possibly some update in the docs. You don't have to explain error messages in docs, but it might be nice to explain correct usage.

@sbillinge I add the news. I can add another issue on adding example/utility for functionalities we updated this PR?

sbillinge · 2024-11-26T14:22:31Z

looks good. this needs a news and possibly some update in the docs. You don't have to explain error messages in docs, but it might be nice to explain correct usage.

@sbillinge I add the news. I can add another issue on adding example/utility for functionalities we updated this PR?

Why not just do it on this PR?

sbillinge

nicely done there! Please see my comments and then I can merge this.

sbillinge · 2024-11-26T18:37:50Z

doc/source/examples/diffractionobjectsexample.rst

+
+    # convert q to tth
+    from diffpy.utils.scattering_objects.diffraction_objects import DiffractionObject
+    test = DiffractionObject(wavelength=1.54)


instead of test let's call it my_diffraction_pattern

We are not writing all of the docs here, so you can start with something like "assuming we have created a DiffractionObject called my_diffraction_pattern from a measured diffraction pattern, and we have specified the wavelenth (see Section ??) we can use the q_to_tth and tth_to_q functions to convert between Q and two-theta by typing my_diffraction_pattern.q_to_tth()..." ..and so on. Let's make an issue to add to docs "How to set a wavelength" and how to instantiate a diffraction object and how to add a diffraction pattern.

sbillinge · 2024-11-26T18:39:59Z

doc/source/utilities/diffractionobjectsutility.rst

+for managing and analyzing diffraction data, including angle-space conversions
+and interactions between diffraction data.
+
+- ``q_to_tth()``: Converts an array of q values to their corresponding two theta values, based on specified wavelength.


we don't need this here. It will be auto-generated in the API docs, and it is shown as an aexample above.

yucongalicechen · 2024-11-26T23:46:25Z

@sbillinge ready for review! I added an issue for additional docs (#183).

sbillinge

I think we don't need these methods at all, at least as public methods. Please see my comment.

sbillinge · 2024-11-26T23:59:28Z

doc/source/examples/diffractionobjectsexample.rst

-   To load the converted array, you can either call ``test.q_to_tth()`` or ``test.on_q[0]``.
-
-   Similarly, use the function ``tth_to_q`` to convert two theta values in degrees to q values. ::
+   To load the converted array, you can either call ``test.q_to_tth()`` or ``test.on_q[0]``. ::


this is confusing. Doesn't this function do it in place and just set the tth array? In this respect, shouldn't this just be a private function and not used by the user at all? In other words, I would load my diffraction data into the object and the object automatically populates all the different arrays. So to get my data on q if I loaded it on tth I would just do my_diffraction_data.on_q

I think this is a private function for now (I didn't see the "_" in the function name)? We need to call insert_scattering_quantity in order for it to populate on all arrays automatically. If they just do my_diffraction_pattern = DiffractionObjects() and my_diffraction_pattern.on_q = ... it is not automatically populated to tth.

but if they do that they are just instantiating an empty DO so there is nothing to populate anywhere..... So that is desired behavior.

The question is what behavior do we want? I want to be able to get my intensity data on all the different x-grids, but I can do that by typing my_pattern.on_q orwhatever. What is the UC where I would want to run that function?

I see. Does this sound better?

Assuming we have created a DiffractionObject called my_diffraction_pattern from a measured diffraction pattern, and we have specified the wavelength (see Section ??, to be added), we can use the q_to_tth and tth_to_q functions to convert between q and two-theta. For example, my_diffraction_pattern.q_to_tth() converts q to two-theta, while my_diffraction_pattern.tth_to_q() converts two-theta to q. The converted array can be accessed using by calling my_diffraction_pattern.on_q[0] for q, or my_diffraction_pattern.on_tth[0] for two-theta.

yucongalicechen added 2 commits November 14, 2024 23:08

fix docstring and add error messages

7841ff2

add tests

1b0fa19

yucongalicechen commented Nov 15, 2024

View reviewed changes

src/diffpy/utils/scattering_objects/diffraction_objects.py Show resolved Hide resolved

sbillinge reviewed Nov 16, 2024

View reviewed changes

edit error message

4e85b0a

yucongalicechen commented Nov 20, 2024

View reviewed changes

sbillinge reviewed Nov 21, 2024

View reviewed changes

add more tests

a33482b

sbillinge reviewed Nov 22, 2024

View reviewed changes

add warning msg for wavelength, edit error msg

3efde28

sbillinge reviewed Nov 25, 2024

View reviewed changes

edit warning msg for wavelength, change index error to runtime error

a53461f

add functionality for handling error messages

2a5a819

sbillinge reviewed Nov 26, 2024

View reviewed changes

remove docstring from private test functions

64d8693

sbillinge reviewed Nov 26, 2024

View reviewed changes

add news

728ff36

add example and utility

490486c

sbillinge reviewed Nov 26, 2024

View reviewed changes

edits on docs

d481253

yucongalicechen mentioned this pull request Nov 26, 2024

docs for diffraction objects setup #183

Closed

sbillinge reviewed Nov 27, 2024

View reviewed changes

sbillinge merged commit d3f83b2 into diffpy:main Nov 28, 2024
5 checks passed

yucongalicechen deleted the q-tth branch December 7, 2024 01:26

yucongalicechen mentioned this pull request Dec 13, 2024

doc for transforms (q_to_tth, tth_to_q, etc.) #226

Closed

q_to_tth & tth_to_q #178

q_to_tth & tth_to_q #178

Conversation

yucongalicechen commented Nov 15, 2024

codecov bot commented Nov 15, 2024 • edited Loading

Codecov Report

sbillinge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbillinge left a comment

Choose a reason for hiding this comment

yucongalicechen commented Nov 21, 2024

sbillinge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yucongalicechen commented Nov 25, 2024

sbillinge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yucongalicechen commented Nov 25, 2024

sbillinge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbillinge left a comment

Choose a reason for hiding this comment

yucongalicechen commented Nov 26, 2024

sbillinge commented Nov 26, 2024

sbillinge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yucongalicechen commented Nov 26, 2024

sbillinge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 15, 2024 •

edited

Loading