test: created a test for squeeze morph using sine wave. Created funct… #178
Conversation
|
I forgot to write the News section! |
|
Just added news, I think it worked |
|
Just updated the code to do the cubic and quadratic non-linear squeeze to the x-axis. I have updated the code to have squeeze_1 and squeeze_2. I have also tested the code (hopefully correctly?) |
|
The function now applies a non-linear transformation to the x-axis (x + squeeze_1x^2 + squeeze_2x^3). The results are the same y-axis values in a non-linearly transformed x-axis. Would be good to plot the results in the test to check how it looks. |
|
Will need to interpolate if target data has different x-axis values than the data we want to morph. For our current experiment, the target data goes up to 22 1/A and the data we want to morph goes up to ~11 1/A. We want to interpolate the target data to the morph data. |
There was a problem hiding this comment.
great start. Please see my inline comments.
My main comment is that we may be going too fast here. I don't see a discussion of what we want to accomplish here and then clear tests of that. I am not sure if you are using ChatGPT but it is looking a bit like what would come from there. We don't really want to defer architectural decisions to chatgpt, though it can be useful as a first cut to specific code blocks.
We want to make sure to be consistent across morphs. There is always a choice between keeping the x-array constant and varying the y-array or vice versa. the stretch morph seems to be transforming on the y-array, so I think it may be a bad idea to transform the x-array here.
to be confident it is working I would go all the way of creating an input that is squeezed but on a uniform grid, because that is the data that we will be getting. I.e., I imagine that we will do an experiment that will give a uniform Q-grid, but because of a weird calibration boo boo, the signal has been squeezed. The transform we want to do is to unsqueeze it so we can compare it to synchrotron data that has no such calibration error. But in general we may want to operate on those arrays (like take a difference etc.) so we would like them to be on the same grid as each other.
Just some thoughts.
If this is what we want, let's devise a test that does this. Then the code.
|
|
||
| **Changed:** | ||
|
|
||
| * Added cubic and quadratic terms, as well as |
There was a problem hiding this comment.
none of these are needed. We just need one news item per PR basically, not a history of everything that happened on the way. So what is needed is, under Added, something like "Squeeze morph that carries out a non-linear stretch to the independent variable axis."
| from diffpy.morph.morphs.morphsqueeze import MorphSqueeze | ||
|
|
||
|
|
||
| class TestMorphSqueeze: |
There was a problem hiding this comment.
What is your reason for making a class here? We usually just write simpler functions but annotate them to show the intended behavior. Please see diffpy.utils for examples
There was a problem hiding this comment.
I was following test_morphstretch.py. Will change this
|
|
||
| class TestMorphSqueeze: | ||
| @pytest.fixture | ||
| def setup(self): |
There was a problem hiding this comment.
I don't think this needs to be a fixture. If we are going to reuse it in multiple places we can make it a fixture, but then put it in conftest.py
| ) | ||
|
|
||
| # Verify that the morph output matches the original input | ||
| assert np.allclose(x_morph, self.x_morph) |
There was a problem hiding this comment.
this is easier to follow using the expected and actual syntax
| assert res2 < 0.1 | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
|
@sbillinge thanks for the feedback. I was using test_morphstretch.py and morphstretch.py as my basis, that is why I made a class for TestMorphSqueeze, made a fixture, etc. I was changing the x-axis instead of the y-axis as I thought was more intuitive but makes sense to be consistent with the other morphs. I will look at diffpy.utils for examples, I think this will really help me here as I am not used to the syntax or the tests. |
|
@sbillinge Should I create a new PR where I can add a test and we can go step by step? Or continue with this PR? I will generate a test where we have an expected uniform signal, such as a sine wave, our input data will be a squeezed version of the sine wave, and then I will unsqueeze it. |
yes, this is a good approach. |
|
for this first test, I would create the squeezed target and then plot it on top of the unsqueezed one and paste that in the comments on the PR so we have a visual about what it looks like. We won't do any plotting as part of the test, but it is part of the workflow of building a good test that we are confident is testing what we want to test. |
|
closing. replaced by #182 |
I have created a test, test_morphsqueeze.py, and a function, morpsqueeze.py, for squeeze morph. This has been now applied to a sine wave and the test run successfully. I have written the test and the function based on the other morphs. Next step is to change the function to use polynomials. I am not sure how do you go from this new morph class to update the main code? Is this done automatically?