raise ValueError if x_squeezed is not strictly increasing
#248
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #248 +/- ##
=======================================
Coverage 99.92% 99.92%
=======================================
Files 23 23
Lines 1355 1355
=======================================
Hits 1354 1354
Misses 1 1 🚀 New features to boost your workflow:
|
|
@sbillinge, it's ready for review. |
| if not strictly_increasing_x: | ||
| raise ValueError( | ||
| "Computed squeezed x is not strictly increasing. " | ||
| "Please change the input x_morph or the squeeze " |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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."
?
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
@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.
|
See my comment on #249. Also, the error message should indicate that this is a refinement issue, not a user issue. Maybe something along the lines of |
|
Ideally, we don't ever have to throw this error out in the first place. Is there an alternative to |
@Sparks29032 |
|
Non-monotonicity does not necessarily imply duplicated values on a discrete grid, though the issue of duplicated values is definitely important; thanks for pointing that out. My main concern is that this morph often gives this error during refinement on a dense grid since switching two adjacent function values around does not greatly change the Rw, hence giving non-monotonic squeeze values. Is there a way that we can enforce monotonicity during refinement? Or can we correct for non-monotonicity somehow? |
|
Hmm, techniques to do so seem a bit complex for the purposes of |
There was a problem hiding this comment.
@Sparks29032 @sbillinge , it's ready for review.
| strictly_increasing_x = (np.diff(x_squeezed) > 0).all() | ||
| if not strictly_increasing_x: | ||
| raise ValueError( | ||
| "Computed squeezed x is not strictly increasing. " |
There was a problem hiding this comment.
The error message is updated.
There was a problem hiding this comment.
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).
| raise ValueError( | ||
| "Computed squeezed x is not strictly increasing. " | ||
| "The squeezed morph is only intended for small polynomial " | ||
| "stretches. Please decrease the magnitude of the polynomial " |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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.
|
Closed, as we will handle the not-strictly-increasing |
3 participants
What problem does this PR address?
Address the first item in #241.
raise ValueError if
x_squeezedis not strictly increasing.What should the reviewer(s) do?
Please check the implementation.
ifinstead oftryandexceptis used since it is more specific to the failed case. Please see which one do we want to use.