add new features in `scale_to()` so user can run without an x-value #289

yucongalicechen · 2024-12-29T04:08:39Z

closes #287
@sbillinge reafy for review

yucongalicechen · 2024-12-29T04:09:30Z

src/diffpy/utils/diffraction_objects.py

@@ -402,14 +402,16 @@ def scale_to(self, target_diff_object, q=None, tth=None, d=None, offset=0):

        The y-value in the target at the closest specified x-value will be used as the factor to scale to.
        The entire array is scaled by this factor so that one object places on top of the other at that point.
-        If multiple values of `q`, `tth`, or `d` are provided, or none are provided, an error will be raised.
+        If none of `q`, `tth`, or `d` are provided,
+        the scaling will be based on the maximal x-array value from both objects.


add new feature

as above. Put this behavior first.

yucongalicechen · 2024-12-29T04:09:57Z

src/diffpy/utils/diffraction_objects.py


        Parameters
        ----------
        target_diff_object: DiffractionObject
            the diffraction object you want to scale the current one onto

-        q, tth, d : float, optional, must specify exactly one of them
+        q, tth, d : float, optional, default is q with the maximal x-array value of the current object


edit description

codecov · 2024-12-29T04:10:28Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (3f2d0a4) to head (0691e54).
Report is 55 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #289   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            8         8           
  Lines          408       496   +88     
=========================================
+ Hits           408       496   +88

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

... and 2 files with indirect coverage changes

src/diffpy/utils/diffraction_objects.py

yucongalicechen · 2024-12-29T04:12:11Z

tests/test_diffraction_objects.py

-    [
-        # Test expected errors produced from scale_to() with invalid inputs
-        (  # C1: none of q, tth, d, provided, expect ValueError
+        (  # C5: none of q, tth, d, provided, expect to scale on the maximal x-arrays


new test case

put this one first? I think it is worth the extra effort as this is our new default behavior.

sbillinge

Thanks @yucongalicechen !. please see inline comments.

sbillinge · 2024-12-29T12:30:15Z

doc/source/examples/diffraction_objects_example.rst

@@ -117,6 +117,13 @@ For convenience, you can also apply an offset to the scaled new diffraction obje

    scaled_and_offset_measured = measured.scale_to(calculated, q=5.5, offset=0.5)

+You can call `scale_to()` without specifying a value for `q`, `tth`, or `d`.


This will be the default behavior so should go first in the docs. Then we can say something like "if this doesnlt give desirable results you can user....." and describe the other workflow.

To give even more flexibility to the user we may want to allow them to specify a separate xtype and xvalue for the target and the scaled DO. The hierarchy woiuld be:

no xtype/xvalue

if that doesn't give desirable results, specify xtype and xvalue for target. scale to the same (nearest) point in the scaled pattern

if that doesn't give desirable results, specify xtype, and a different xvalue for target. and scaled DO, respectively. Scale the yvale of xvalue_scaled to yvalue of xvalue_target.

This last functionality is a new one and we should put it on a new issue (and a new PR if we want to implement it)

sbillinge · 2024-12-29T12:31:07Z

news/scaleto-max.rst

@@ -0,0 +1,23 @@
+**Added:**
+
+* new feature in `scale_to()` to run without specifying an x-value


describe what it does, not what it doesn't do...i.e., scales to max value in each chase. You can also add that it allows this to be used without xvalue

sbillinge · 2024-12-29T12:31:26Z

src/diffpy/utils/diffraction_objects.py

@@ -402,14 +402,16 @@ def scale_to(self, target_diff_object, q=None, tth=None, d=None, offset=0):

        The y-value in the target at the closest specified x-value will be used as the factor to scale to.
        The entire array is scaled by this factor so that one object places on top of the other at that point.
-        If multiple values of `q`, `tth`, or `d` are provided, or none are provided, an error will be raised.
+        If none of `q`, `tth`, or `d` are provided,
+        the scaling will be based on the maximal x-array value from both objects.


as above. Put this behavior first.

sbillinge · 2024-12-29T12:32:03Z

tests/test_diffraction_objects.py

-    [
-        # Test expected errors produced from scale_to() with invalid inputs
-        (  # C1: none of q, tth, d, provided, expect ValueError
+        (  # C5: none of q, tth, d, provided, expect to scale on the maximal x-arrays


put this one first? I think it is worth the extra effort as this is our new default behavior.

sbillinge · 2024-12-29T12:33:46Z

tests/test_diffraction_objects.py

@@ -319,8 +303,25 @@ def test_scale_to(org_do_args, target_do_args, scale_inputs, expected):
                "d": None,
                "offset": 0,


this is a different issue, unrelated to what you are doing here, but I would rather have default offset to be None (set to zero if None). I think this is more logical for my brain. If I don't set an offset, the offset is None.

sbillinge · 2024-12-29T12:36:42Z

tests/test_diffraction_objects.py

+    original_do = DiffractionObject(**org_do_args)
+    target_do = DiffractionObject(**target_do_args)
+    scaled_do = original_do.scale_to(
+        target_do, q=scale_inputs["q"], tth=scale_inputs["tth"], d=scale_inputs["d"], offset=scale_inputs["offset"]


I think we need to fgure out a way only pass in what is needed, rather than passing in everything with some things set to default values. It may need more test functions written, but see if you can figure out how to do it with unpacking kwargs that are handed in.

… update tests for flexible inputs

yucongalicechen · 2024-12-29T22:02:16Z

@sbillinge ready for review

doc/source/examples/diffraction_objects_example.rst

news/scaleto-max.rst

src/diffpy/utils/diffraction_objects.py

tests/test_diffraction_objects.py

sbillinge

small tweaks

sbillinge · 2024-12-30T01:47:22Z

src/diffpy/utils/diffraction_objects.py

        If multiple values of `q`, `tth`, or `d` are provided, an error will be raised.

        Parameters
        ----------
        target_diff_object: DiffractionObject
            the diffraction object you want to scale the current one onto

-        q, tth, d : float, optional, default is q with the maximal x-array value of the current object
+        q, tth, d : float, optional, default is the max q-value from each object


"default is None"

src/diffpy/utils/diffraction_objects.py

sbillinge · 2024-12-30T01:48:54Z

src/diffpy/utils/diffraction_objects.py

            an offset to add to the scaled y-values

        Returns
        -------
        the rescaled DiffractionObject as a new object
        """
+        if offset is None:
+            offset = 0
+


remove empty line

sbillinge

please see inline. I answered a question, and raised a new one.

src/diffpy/utils/diffraction_objects.py

tests/test_diffraction_objects.py

…to scaleto2

src/diffpy/utils/diffraction_objects.py

tests/test_diffraction_objects.py

yucongalicechen · 2024-12-30T03:25:19Z

@sbillinge ready for review!

yucongalicechen added 3 commits December 28, 2024 23:02

add news

e91661d

add example

4a87d00

add feature so that user can run scale_to without xvalue

2c8335c

yucongalicechen commented Dec 29, 2024

View reviewed changes

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

yucongalicechen commented Dec 29, 2024

View reviewed changes

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

yucongalicechen commented Dec 29, 2024

View reviewed changes

sbillinge reviewed Dec 29, 2024

View reviewed changes

yucongalicechen mentioned this pull request Dec 29, 2024

feat: allow separate xvalues for self and target objects in scale_to() #290

Open

yucongalicechen added 3 commits December 29, 2024 16:28

refactor: reorder docstring and tests, change offset default to None,…

630011a

… update tests for flexible inputs

docs: clarify docstring and news

66c67c6

docs: edit example file

0a155aa

yucongalicechen commented Dec 29, 2024

View reviewed changes

doc/source/examples/diffraction_objects_example.rst Show resolved Hide resolved

yucongalicechen commented Dec 29, 2024

View reviewed changes

news/scaleto-max.rst Show resolved Hide resolved

yucongalicechen commented Dec 29, 2024

View reviewed changes

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

yucongalicechen commented Dec 29, 2024

View reviewed changes

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

yucongalicechen commented Dec 29, 2024

View reviewed changes

tests/test_diffraction_objects.py Show resolved Hide resolved

sbillinge reviewed Dec 30, 2024

View reviewed changes

yucongalicechen and others added 2 commits December 29, 2024 21:04

docs: improve docstring

13f227d

doc: small tweak to description of scale_to

cdf4b49

sbillinge reviewed Dec 30, 2024

View reviewed changes

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

tests/test_diffraction_objects.py Outdated Show resolved Hide resolved

yucongalicechen added 3 commits December 29, 2024 22:19

feat: add offset to default behavior and test case

27da4de

Merge branch 'scaleto2' of github.com:yucongalicechen/diffpy.utils in…

8d7b540

…to scaleto2

docs: improve docstring and test comment

0691e54

yucongalicechen commented Dec 30, 2024

View reviewed changes

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

yucongalicechen commented Dec 30, 2024

View reviewed changes

tests/test_diffraction_objects.py Show resolved Hide resolved

sbillinge merged commit 88d8d73 into diffpy:main Dec 30, 2024
5 checks passed

yucongalicechen deleted the scaleto2 branch December 30, 2024 15:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add new features in `scale_to()` so user can run without an x-value #289

add new features in `scale_to()` so user can run without an x-value #289

yucongalicechen commented Dec 29, 2024 •

edited

Loading

yucongalicechen Dec 29, 2024

sbillinge Dec 29, 2024

yucongalicechen Dec 29, 2024

codecov bot commented Dec 29, 2024 •

edited

Loading

yucongalicechen Dec 29, 2024

sbillinge Dec 29, 2024

sbillinge left a comment

sbillinge Dec 29, 2024

sbillinge Dec 29, 2024

sbillinge Dec 29, 2024

sbillinge Dec 29, 2024

sbillinge Dec 29, 2024

sbillinge Dec 29, 2024

yucongalicechen commented Dec 29, 2024

sbillinge left a comment

sbillinge Dec 30, 2024

sbillinge Dec 30, 2024

sbillinge left a comment

yucongalicechen commented Dec 30, 2024

		@@ -117,6 +117,13 @@ For convenience, you can also apply an offset to the scaled new diffraction obje

		scaled_and_offset_measured = measured.scale_to(calculated, q=5.5, offset=0.5)

		You can call `scale_to()` without specifying a value for `q`, `tth`, or `d`.

		@@ -0,0 +1,23 @@
		Added:

		* new feature in `scale_to()` to run without specifying an x-value

		@@ -319,8 +303,25 @@ def test_scale_to(org_do_args, target_do_args, scale_inputs, expected):
		"d": None,
		"offset": 0,

add new features in scale_to() so user can run without an x-value #289

add new features in scale_to() so user can run without an x-value #289

Conversation

yucongalicechen commented Dec 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 29, 2024 • edited Loading

Codecov Report

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

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 Dec 29, 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

sbillinge left a comment

Choose a reason for hiding this comment

yucongalicechen commented Dec 30, 2024

add new features in `scale_to()` so user can run without an x-value #289

add new features in `scale_to()` so user can run without an x-value #289

yucongalicechen commented Dec 29, 2024 •

edited

Loading

codecov bot commented Dec 29, 2024 •

edited

Loading