-
Notifications
You must be signed in to change notification settings - Fork 17
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
Changes from 7 commits
9a53b24
ac388e9
ce863d3
ca69295
a1b6425
14d92cb
a48ce43
d42d234
f47e167
4933b4f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
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: | ||
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
import numpy as np | ||
import pytest | ||
from numpy.polynomial import Polynomial | ||
from scipy.interpolate import interp1d | ||
|
||
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, the full polynomial is applied | ||
[0, 0.03, 0, -0.001], | ||
# Testing zeros, expect no squeezing | ||
[0, 0, 0, 0, 0, 0], | ||
], | ||
) | ||
def test_morphsqueeze(squeeze_coeffs): | ||
x_expected = np.linspace(0, 10, 1001) | ||
y_expected = np.sin(x_expected) | ||
x_make = np.linspace(-3, 13, 3250) | ||
squeeze_polynomial = Polynomial(squeeze_coeffs) | ||
x_squeezed = x_make + squeeze_polynomial(x_make) | ||
y_morph = np.sin(x_squeezed) | ||
morph = MorphSqueeze() | ||
morph.squeeze = squeeze_coeffs | ||
x_actual, y_actual, x_target, y_target = morph( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good names may be |
||
x_make, y_morph, x_expected, y_expected | ||
) | ||
y_actual = interp1d(x_actual, y_actual)(x_target) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. interp1d is deprecated. we shouldn't use it. If you google this you will find recommendations for alternative approaches. Basically using cubic splines is probably best. Also, this is not Of course the test will fail until you implement it in the function, but that is the point of the test. We first write tests that capture the behavior that we want but that fail. Then we write the code until the tests pass. When the tests pass, then the code we have written captures the behavior we want by definition. But ONLY if we were very strict that the wrote the test that captured the behavior we want. |
||
x_actual = x_target | ||
assert np.allclose(y_actual, y_expected) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like there is already a MorphRGrid There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ... There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
You are doing really well. Writing good code is hard, but worth it...... Sorry I am making it so slow by being so picky! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 :) |
||
assert np.allclose(x_actual, x_expected) | ||
assert np.allclose(x_target, x_expected) | ||
assert np.allclose(y_target, y_expected) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think these tests are quite right. they should all look like:
Then we hard-code the expected's to be what we want |
||
|
||
# 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_make, y_morph, color='purple', label='morph') | ||
# plt.plot(x_actual, y_actual, '--', color='gold', label='Actual') | ||
# plt.legend() | ||
# plt.show() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a comment here on why this grid was chosen for the morph. (a) we want it to be different than the target grid in general (b) we want to support the case where the morph grid is finer than the target but not vice versa.
In general we want to test the behavior but at minimal computational cost, so we may want to coarsen all the grids...maybe even by 10x.