feat: add warning for extrapolation in `morphsqueeze` #250

ycexiao · 2025-08-25T18:28:42Z

What problem does this PR address?

Address the issue mentioned in #243. Create a warning for extrapolation in morphsqeeze.

What should the reviewer(s) do?

Please check the implementation.

codecov · 2025-08-25T18:32:20Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.92%. Comparing base (3908d63) to head (e165b41).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #250   +/-   ##
=======================================
  Coverage   99.92%   99.92%           
=======================================
  Files          23       23           
  Lines        1318     1354   +36     
=======================================
+ Hits         1317     1353   +36     
  Misses          1        1

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ycexiao

@sbillinge @Sparks29032, it's ready for review.

ycexiao · 2025-08-25T18:33:59Z

src/diffpy/morph/morphs/morphsqueeze.py

        self.extrap_index_low = low_extrap[-1] if low_extrap.size else None
        self.extrap_index_high = high_extrap[0] if high_extrap.size else None
+
+        low_extrap_x = x_squeezed[0] - self.x_morph_in[0]


I think the length of r being extrapolated is more direct than the number of points being extrapolated.

I think the length of r being extrapolated is more direct than the number of points being extrapolated.

The number of points is probably better. Let's say the user is told that the lower 10 points and upper 20 are being extrapolated. Then, they can simply take array[10:-20] if they do not want to work with extrapolated data.

See most recent comment.

I think the best would just be to say something like: "Warning: all points below {low} and above {high} are extrapolated using cubic spline etc. etc."

Make it clear that the low and high thresholds are for the grid (can't think of a proper way to word it right now).

Best to mention the method of extrapolation used (cubic spline I believe).

Edit for clarity: The low and high shouldn't be the lengths currently reported but rather the locations above/below which extrapolation is happening.

ycexiao · 2025-08-25T18:39:49Z

I will add a test for it if we are sure this implementation is what we want.

Sparks29032 · 2025-08-25T18:40:20Z

src/diffpy/morph/morphs/morphsqueeze.py

+        if low_extrap_x > 0 or high_extrap_x > 0:
+            wmsg = "Extrapolating the morphed function: "
+            if low_extrap_x > 0:
+                wmsg += f"extrapolating length in the lowe r {low_extrap_x} "


@Sparks29032 is the implement ok, typo aside? Then @ycexiao can write a test.

I'd prefer the grid values above/below which extrapolation occur be reported rather than the extrapolation length since the former quantity if probably more useful (made a comment above about this).

Sparks29032 · 2025-08-26T02:01:50Z

"Warning: Points with grid values below {begin_end_sqeeze[0]} and above {begin_end_sqeeze[1]} will be extrapolated via cubic spline."

@ycexiao Maybe something like this? No capitals since we are not talking about the Python class, but rather the technique. Also make clear it is the grid. We don't need to tell them that we are obtaining points between x and y as a user of squeeze should already know this from the docs or manual.

Sparks29032 · 2025-08-26T02:03:19Z

Can you also test it for the CLI case? Thanks

ycexiao

@Sparks29032 @sbillinge, it's ready for review.

ycexiao · 2025-08-26T02:02:39Z

src/diffpy/morph/morphs/morphsqueeze.py

            self.x_morph_in
        )
        low_extrap = np.where(self.x_morph_in < x_squeezed[0])[0]
        high_extrap = np.where(self.x_morph_in > x_squeezed[-1])[0]


Do we want to remove these few lines of code? I can't find other references to the variables defined here.

These were variables we made before to store what you have now called begin_end_squeeze. You can delete it if you prefer using begin_end_squeeze.

I find these variables provide a handy interface to test the extrapolated effect in test_morphsqueeze, so I think we should keep them.

ycexiao · 2025-08-26T02:03:28Z

src/diffpy/morph/morphs/morphsqueeze.py

+            begin_end_sqeeze[0] <= begin_end_in[0]
+            and begin_end_in[-1] >= begin_end_in[-1]
+        ):
+            wmsg = (


Output effect:

Can you make it say Warning: on the CLI as well?

Sparks29032 · 2025-08-26T02:06:43Z

I liked the behavior before where you only say extrapolatipn below if there is extrapolation below and above if there is above. So if only the above side has the warning:

Warning: Points with grid values above {begin_end_sqeeze[1]} will be extrapolated via cubic spline.

ycexiao · 2025-08-26T02:09:43Z

We don't need to tell them that we are obtaining points between x and y, as a user of Squeeze should already know this from the docs or manual.

I think it would be helpful to reduce the bandwidth for users when they are tuning the coefficient to choose a moderate extrapolation length if we have both the expected x range and actual x range. Or do we prefer simplicity here?

Sparks29032 · 2025-08-26T02:12:41Z

The expected x range is just the target grid range. Also they cannot choose the extrapolation length, the refinement upon compression does so. There is no variable or user input to choose where they want to extrapolate.

When the user plots or obtains the data, all they need to know is where are the cutoff where data is extrapolated. Thus, if wonky behavior occurs in those ranges, they know why.

Imo, the point of this warning is to tell the user where the function will fail to do the expected behavior, not tell the user the expected behavior.

ycexiao · 2025-08-26T02:15:35Z

Thank you! Good to know.

ycexiao

@Sparks29032 @sbillinge, it's ready for review.

ycexiao · 2025-08-26T03:48:00Z

src/diffpy/morph/morphs/morphsqueeze.py

            self.x_morph_in
        )
        low_extrap = np.where(self.x_morph_in < x_squeezed[0])[0]
        high_extrap = np.where(self.x_morph_in > x_squeezed[-1])[0]


I find these variables provide a handy interface to test the extrapolated effect in test_morphsqueeze, so I think we should keep them.

ycexiao · 2025-08-26T03:51:43Z

tests/test_morphsqueeze.py

+    assert expected_wmsg in result.stderr
+
+
+def create_morph_data_file(


Do we want to create a helper.py module in \tests? If we want to add more CLI tests, we might need a handy function to create temporary *.cgr files. We can write this function in helper.py, and other tests can import it.

That would be helpful! (see #233)

ycexiao · 2025-08-26T03:56:40Z

CLI Warning output:

ycexiao and others added 2 commits August 25, 2025 14:28

feat: add warning for extrapolation in morphsqueeze

400851b

[pre-commit.ci] auto fixes from pre-commit hooks

4f59c54

chore: add news

bdd29ae

ycexiao commented Aug 25, 2025

View reviewed changes

ycexiao marked this pull request as ready for review August 25, 2025 18:38

Sparks29032 reviewed Aug 25, 2025

View reviewed changes

test: add test for warning extrapolation in morphsqueeze

0cbcee4

ycexiao commented Aug 26, 2025

View reviewed changes

ycexiao added 2 commits August 25, 2025 23:07

chore: update warning message for extrapolation in morphsqueeze.py

026984a

test: add CLI test for extrapolation warning

e165b41

ycexiao commented Aug 26, 2025

View reviewed changes

sbillinge merged commit 3c52108 into diffpy:main Aug 26, 2025
5 checks passed

This was referenced Sep 2, 2025

Remove subprocess calls from tests #251

Merged

Raise warnings for extreme extrapolation or interpolation #243

Closed

Only warn user about squeeze extrapolation after refinement #252

Merged

Sparks29032 mentioned this pull request Sep 30, 2025

Add user input tolerance and extrapolation warning to MorphSqueeze #183

Closed

		assert expected_wmsg in result.stderr


		def create_morph_data_file(

feat: add warning for extrapolation in morphsqueeze #250

feat: add warning for extrapolation in morphsqueeze #250

Uh oh!

Conversation

ycexiao commented Aug 25, 2025

What problem does this PR address?

What should the reviewer(s) do?

Uh oh!

codecov bot commented Aug 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ycexiao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sparks29032 Aug 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sparks29032 Aug 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ycexiao commented Aug 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sparks29032 commented Aug 26, 2025

Uh oh!

Sparks29032 commented Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ycexiao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sparks29032 Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sparks29032 commented Aug 26, 2025

Uh oh!

ycexiao commented Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sparks29032 commented Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ycexiao commented Aug 26, 2025

Uh oh!

ycexiao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ycexiao commented Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

feat: add warning for extrapolation in `morphsqueeze` #250

feat: add warning for extrapolation in `morphsqueeze` #250

codecov bot commented Aug 25, 2025 •

edited

Loading

Sparks29032 Aug 25, 2025 •

edited

Loading

Sparks29032 Aug 25, 2025 •

edited

Loading

Sparks29032 commented Aug 26, 2025 •

edited

Loading

Sparks29032 Aug 26, 2025 •

edited

Loading

ycexiao commented Aug 26, 2025 •

edited

Loading

Sparks29032 commented Aug 26, 2025 •

edited

Loading

ycexiao commented Aug 26, 2025 •

edited

Loading