Skip to content

feat: add warning for extrapolation in morphsqueeze #250

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sbillinge merged 6 commits into from
Aug 26, 2025
Merged

feat: add warning for extrapolation in morphsqueeze #250

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
400851b
feat: add warning for extrapolation in `morphsqueeze`
ycexiao Aug 25, 2025
4f59c54
[pre-commit.ci] auto fixes from pre-commit hooks
pre-commit-ci[bot] Aug 25, 2025
bdd29ae
chore: add news
ycexiao Aug 25, 2025
0cbcee4
test: add test for warning extrapolation in `morphsqueeze`
ycexiao Aug 26, 2025
026984a
chore: update warning message for extrapolation in `morphsqueeze.py`
ycexiao Aug 26, 2025
e165b41
test: add CLI test for extrapolation warning
ycexiao Aug 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/diffpy/morph/morphs/morphsqueeze.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
"""Class MorphSqueeze -- Apply a polynomial to squeeze the morph
function."""

import warnings

import numpy as np
from numpy.polynomial import Polynomial
from scipy.interpolate import CubicSpline
Expand Down Expand Up @@ -85,4 +87,17 @@ def morph(self, x_morph, y_morph, x_target, y_target):
high_extrap = np.where(self.x_morph_in > x_squeezed[-1])[0]
Copy link
Copy Markdown
Collaborator Author

@ycexiao ycexiao Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator

@Sparks29032 Sparks29032 Aug 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

@ycexiao ycexiao Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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]
Copy link
Copy Markdown
Collaborator Author

@ycexiao ycexiao Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator

@Sparks29032 Sparks29032 Aug 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

@Sparks29032 Sparks29032 Aug 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

high_extrap_x = self.x_morph_in[-1] - x_squeezed[-1]
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} "
Copy link
Copy Markdown
Collaborator

@Sparks29032 Sparks29032 Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo?

Copy link
Copy Markdown
Contributor

@sbillinge sbillinge Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator

@Sparks29032 Sparks29032 Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if high_extrap_x > 0:
wmsg += f"extrapolating length in the high r {high_extrap_x} "
warnings.warn(
wmsg,
UserWarning,
)
return self.xyallout
Loading