Skip to content

raise ValueError if x_squeezed is not strictly increasing #248

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

Closed
ycexiao wants to merge 3 commits into from
Closed

raise ValueError if x_squeezed is not strictly increasing #248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d6b320d
raise `ValueError` if `x_squeezed` is not strictly increasing
ycexiao Aug 18, 2025
0bc478a
Merge branch 'main' into strictly-increasing-squeeze
ycexiao Sep 11, 2025
add0413
chore: update error message
ycexiao Sep 11, 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
23 changes: 23 additions & 0 deletions news/strictly-increasing-squeeze.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
**Added:**

* Raise ``ValueError`` if ``x_squeezed`` is not strictly increasing.

**Changed:**

* <news item>

**Deprecated:**

* <news item>

**Removed:**

* <news item>

**Fixed:**

* <news item>

**Security:**

* <news item>
9 changes: 9 additions & 0 deletions src/diffpy/morph/morphs/morphsqueeze.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,15 @@ def morph(self, x_morph, y_morph, x_target, y_target):
coeffs = [self.squeeze[f"a{i}"] for i in range(len(self.squeeze))]
squeeze_polynomial = Polynomial(coeffs)
x_squeezed = self.x_morph_in + squeeze_polynomial(self.x_morph_in)
strictly_increasing_x = (np.diff(x_squeezed) > 0).all()
if not strictly_increasing_x:
raise ValueError(
"Computed squeezed x is not strictly increasing. "
Copy link
Collaborator Author

@ycexiao ycexiao Sep 11, 2025

Choose a reason for hiding this comment

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

The error message is updated.

Copy link
Contributor

@sbillinge sbillinge Sep 12, 2025

Choose a reason for hiding this comment

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

I added a suggested error message on the main comment thread. I think the non-strictly increasing problem probably emerges after the squeeze regression runs, so it may not matter what starting parameters the user gives (if the regression is working as hoped).

"The squeezed morph is only intended for small polynomial "
"stretches. Please decrease the magnitude of the polynomial "
Copy link
Contributor

@sbillinge sbillinge Sep 13, 2025

Choose a reason for hiding this comment

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

this still seems pretty cryptic to me. If I got this message as a user I wouldn't know what to change if I ran again. there is agreat discussion on the comment thread which is way clearer. Why don't we capture some of that here. It is ok if the message is long. @Sparks29032 please could you draft something. Tell the whole story, including the "trick" of getting good starting values with squeeze and hshift and using for a0 and a1.

Btw, the fact that this works seems to suggest that it is often a convergence issue and not, in those cases, a fundamental issue, i.e., the regression is stuck in a local minimum far from the right solution when this error happens, so we can mention that too?

Copy link
Collaborator

@Sparks29032 Sparks29032 Sep 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@sbillinge The story I would tell is only empirical from the limited times I've used this morph:

Error: The polynomial applied by the squeeze morph has resulted in a grid that is no longer strictly increasing, likely due to a convergence issue. A strictly increasing grid is required for diffpy.morph to compute the morphed function through cubic spline interpolation. Here are some suggested methods to resolve this:
(1) Please decrease the order of your polynomial and try again.
(2) If you are using initial guesses of all 0, please ensure your objective function only requires a small polynomial squeeze to match your reference. (In other words, there is good agreement between the two functions.)
(3) If you expect a large polynomial squeeze to be needed, please ensure your initial parameters for the polynomial morph result in good agreement between your reference and objective functions. One way to obtain such parameters is to first apply a --hshift and --stretch morph. Then, use the hshift parameter for a0 and stretch parameter for a1.

However, I think we should allow squeeze to obtain non-monotonic values. See #256 for implementation.

"coefficients."
)

self.squeeze_cutoff_low = min(x_squeezed)
self.squeeze_cutoff_high = max(x_squeezed)
self.y_morph_out = CubicSpline(x_squeezed, self.y_morph_in)(
Expand Down
Loading