Skip to content

test: adding a test to unsqueeze squeezed data #180

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.

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
wants to merge 10 commits into from
23 changes: 23 additions & 0 deletions news/test_squeeze.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
**Added:**

* Polynomial squeeze of x-axis of morphed data

**Changed:**

* <news item>

**Deprecated:**

* <news item>

**Removed:**

* <news item>

**Fixed:**

* <news item>

**Security:**

* <news item>
43 changes: 43 additions & 0 deletions src/diffpy/morph/morphs/morphsqueeze.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import numpy as np
from numpy.polynomial import Polynomial
from scipy.interpolate import interp1d

from diffpy.morph.morphs.morph import LABEL_GR, LABEL_RA, Morph


class MorphSqueeze(Morph):
"""Squeeze the morph function.

This applies a polynomial to squeeze the morph non-linearly.

Configuration Variables
-----------------------
squeeze
list or array-like
Polynomial coefficients [a0, a1, ..., an] for the squeeze function.
"""

# Define input output types
summary = "Squeeze morph by polynomial shift"
xinlabel = LABEL_RA
yinlabel = LABEL_GR
xoutlabel = LABEL_RA
youtlabel = LABEL_GR
parnames = ["squeeze"]

def morph(self, x_morph, y_morph, x_target, y_target):
Morph.morph(self, x_morph, y_morph, x_target, y_target)
if self.squeeze is None or np.allclose(self.squeeze, 0):
self.x_morph_out = self.x_morph_in
self.y_morph_out = self.y_morph_in
return self.xyallout

squeeze_polynomial = Polynomial(self.squeeze)
x_squeezed = self.x_morph_in + squeeze_polynomial(self.x_morph_in)

self.y_morph_out = interp1d(
x_squeezed, self.y_morph_in, kind="cubic", bounds_error=False
)(self.x_morph_in)

self.x_morph_out = self.x_morph_in
return self.xyallout
60 changes: 60 additions & 0 deletions tests/test_morphsqueeze.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
import numpy as np
import pytest
from numpy.polynomial import Polynomial

from diffpy.morph.morphs.morphsqueeze import MorphSqueeze


@pytest.mark.parametrize(
"squeeze_coeffs",
[
# The order of coefficients is [a0, a1, a2, ..., an]
# Negative cubic squeeze coefficients
[-0.2, -0.01, -0.001, -0.001],
# Positive cubic squeeze coefficients
[0.2, 0.01, 0.001, 0.001],
# Positive and negative cubic squeeze coefficients
[0.2, -0.01, 0.002, -0.001],
# Quadratic squeeze coefficients
[-0.2, 0.005, -0.007],
# Linear squeeze coefficients
[0.1, 0.3],
# 4th order squeeze coefficients
[0.2, -0.01, 0.001, -0.001, 0.0004],
# Zeros and non-zeros, expect 0 + a1x + 0 + a3x**3
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not quite what I am after. I have a feeling that if a morph parameter is zero the morph is not refined (though I am not sure). Otherwise there would be no need in, e.g., MorphStretch to avoid instantiating the morph object for a zero stretchy, you could just pass it to the morph with a value zero and it would return an unstretched morph.

This is bad coding tbh (wrist slap to my former students), it is what we call "magic" which is always to be avoided. Kind of "hidden behavior" if you like, which is why is like magic. We need to check if this is the case, but if it is the case, then we need to handle it here. So the comment would be something like # Zeros and non-zeros, zero coefficients will not be refined or sthg like that. This will be hard in our case tbh and may break the existing structure of the code in which case we may need to scale back. argh, it is hard to write good code.....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I didn’t consider the implications for refinement. I think the cleanest and most consistent behavior is to only skip the morphing if squeeze is None. If the coefficients are all zero (e.g., [0, 0, 0]), the code should still run. This way, those coefficients remain part of the refinement process and aren’t treated as a special case.
This should avoid any hidden or "magic" behavior and keep everything explicit. So the comment should be like # Zeros and non-zeros, the full polynomial is applied ...
Let me know what you think, I can change the code accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't think any user will ever want to fix one of the coefficients but tefine the others? Remember, the test captures desired behavior, not behavior that is ready to code.... We can decide not to implement things that are to hard, but that decision cubes later when we figure out it is desired but to hard

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think someone might want to fix some coefficients and refine others. I need to think more on how to implement it. So far what I thought we could do is instead of having a squeeze parameter that is an array of coefficients, we could have different input squeeze parameters, squeeze_0, squeeze_1, etc, for each coefficient. Then we could either give them an initial value if we want to refine that coefficient or set it up as None and won't be refined. This is similar as just having different morphs for each coefficient, like shift, stretch, and add more.

[0, 0.03, 0, -0.001],
# Testing zeros, expect no squeezing
[0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
],
)
def test_morphsqueeze(squeeze_coeffs):
x_target = np.linspace(0, 10, 1001)
y_target = np.sin(x_target)

Copy link
Contributor

Choose a reason for hiding this comment

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

close up. no blank line here (and everywhere below)

x_make = np.linspace(-3, 13, 1601)
lower_idx = np.where(x_make == 0.0)[0][0]
upper_idx = np.where(x_make == 10.0)[0][0]

squeeze_polynomial = Polynomial(squeeze_coeffs)
x_squeezed = x_make + squeeze_polynomial(x_make)

x_morph = x_make.copy()
y_morph = np.sin(x_squeezed)

morph = MorphSqueeze()
morph.squeeze = squeeze_coeffs

x_actual, y_actual, x_expected, y_expected = morph(
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not right. You can't have an "expected" returned by the function. x_expected and y_expected are what you are calling x_target and y_target I think. I would just rename them from the beginning.

x_morph, y_morph, x_target, y_target
)
y_actual = y_actual[lower_idx : upper_idx + 1]
assert np.allclose(y_actual, y_expected)
Copy link
Contributor

Choose a reason for hiding this comment

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

we probably want four asserts, one each for the four quantities returned by the function. I think (but again, not sure) that it should simply return a copy of x_target and y_target in those places. We are not actually testing all the possibilities in principle because in our tests the morph and the target are on the same x-grid. Again, a conversation about behavior....do we want to be able to morph things onto each other that are on different grids? If yes, we need to work harder to build our test, but it should be done in the same way.....the expecteds are hard-coded and not coming from the function. Remember that the expecteds encode what behavior we want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ll add the four asserts to check all outputs from the morph.
If we want to keep behavior consistent across all morphs, I think it makes sense for MorphSqueeze to assume same x-grids. But I do think it’s very important to support morphing between datasets on different grids more generally. One idea would be to create a dedicated morph that regrids the data onto a common x-grid before other morphs are applied. This would allow flexibility without overcomplicating individual morph classes? What do you think?
This is especially relevant for workflows like ours where, for example, XFEL data and synchrotron data come in on different grids

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like there is already a MorphRGrid

Copy link
Contributor

Choose a reason for hiding this comment

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

When we make tests it is very important to keep the discussion to desired behavior and not get distracted by how it will be implemented. If the other morphs are all on the same grid we can stick to that but make an issue to address this in the future. Personally I think it would be more useful if they could be on different grids. It won't be hard to implement but some decisions are needed about which grid to use ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a great point. I have focused on the desired behavior in the test and used different grids. I am keeping the target grid fixed and interpolate the morphed data onto it. This works well when the input morph data has a finer step in the x-axis compared to the target data. However, in the scenario where the input morph data has much lower resolution in x-axis the interpolation won't be very accurate. So far I have implemented the interpolation in the test function but we could instead implement it in the MorphSqueeze class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my last commit for the updated test that uses different x-grids. I still need to think through the best way to allow selective refinement of squeeze coefficients. Apologies if I’m moving a bit slowly, I’m new to this but learning a lot! Thank you again for all your mentorship and patience!

Copy link
Contributor

Choose a reason for hiding this comment

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

See my last commit for the updated test that uses different x-grids. I still need to think through the best way to allow selective refinement of squeeze coefficients. Apologies if I’m moving a bit slowly, I’m new to this but learning a lot! Thank you again for all your mentorship and patience!

You are doing really well. Writing good code is hard, but worth it...... Sorry I am making it so slow by being so picky!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please keep being as picky as needed! So that I can learn the right/best way to write good code :)


# Plotting code used for figures in PR comments
# https://github.com/diffpy/diffpy.morph/pull/180
# plt.figure()
# plt.scatter(x_expected, y_expected, color='black', label='Expected')
# plt.plot(x_morph, y_morph, color='purple', label='morph')
# plt.plot(x_actual, y_actual, '--', color='gold', label='Actual')
# plt.legend()
# plt.show()