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 1 commit
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>
7 changes: 7 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 @@ -78,6 +78,13 @@ 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).

"Please change the input x_morph or the squeeze "
Copy link
Contributor

@sbillinge sbillinge Aug 19, 2025

Choose a reason for hiding this comment

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

If possible let's have a more helpfule message here. I am a user. OK, I need to change those things but what do I change them to? How do I change them?

Copy link
Collaborator Author

@ycexiao ycexiao Aug 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@sbillinge What about

                "Computed squeezed x is not strictly increasing. "
                "Please change the input x_morph or the coefficients  "
                "so that the polynomial a0 + (a1+1) * x_morph "
                "+ a2 * x_morph**2 + ...  is strictly increasing."

?

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.

how about
"The squeeze parameters resulted in an x-array that is not strictly increasing which is nonsensical. Please consider squeezing with a lower-order polynomial or over a narrowing range"

Please chech that what I wrote actually makes sense. @Sparks29032 ?

Copy link
Collaborator

@Sparks29032 Sparks29032 Sep 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@sbillinge I agree with lowering the polynomial order, but narrowing the range may not always help resolve this.

One thing I found works empirically (no theory to back this up, and I doubt there will be) is first using stretch and hshift (since they are monotonic) to obtain a decent fit and using that as a0, a1.

Another thing that definitely helps is ensuring that your two functions are relatively close after applying your initial guess.

Perhaps we change "narrowing range" to "over a region of closer agreement". Need to reword but that kind of idea.

"coefficients."
)
self.y_morph_out = CubicSpline(x_squeezed, self.y_morph_in)(
self.x_morph_in
)
Expand Down
Loading