feat: add warning for extrapolation in `morphsqueeze` by ycexiao · Pull Request #250 · diffpy/diffpy.morph · GitHub

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

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

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.

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

Typo?

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

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

Can you also test it for the CLI case? Thanks

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

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.

Output effect:

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

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.

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?

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.

Thank you! Good to know.

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

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

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)

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

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

+                      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} "

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

                       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]

		@@ -85,4 +94,22 @@ def morph(self, x_morph, y_morph, x_target, y_target):
		high_extrap = np.where(self.x_morph_in > x_squeezed[-1])[0]

		assert expected_wmsg in result.stderr


		def create_morph_data_file(

feat: add warning for extrapolation in `morphsqueeze` by ycexiao · Pull Request #250 · diffpy/diffpy.morph · GitHub

Repository navigation

feat: add warning for extrapolation in morphsqueeze - #250

feat: add warning for extrapolation in morphsqueeze#250sbillinge merged 6 commits intodiffpy:maindiffpy/diffpy.morph:mainfrom ycexiao:extrapolate-warningCopy head branch name to clipboard

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!

Reviewers

Uh oh!

Assignees

Labels

feat: add warning for extrapolation in morphsqueeze#250
sbillinge merged 6 commits into
diffpy:mainfrom
ycexiao:extrapolate-warning

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