Skip to content

Squeeze morph: Adding UCs tests #182

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

Merged
merged 9 commits into from
Apr 19, 2025
Merged

Squeeze morph: Adding UCs tests #182

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c4ed0e6
test: adding test and empty function
Luiskitsu Apr 10, 2025
c59b09e
test: adding test and empty function
Luiskitsu Apr 10, 2025
2320b82
test: adding different UCs tests while keeping function empty
Luiskitsu Apr 11, 2025
ead3248
test: writting better docstrings and comments
Luiskitsu Apr 11, 2025
924872e
test: writting better docstrings and comments
Luiskitsu Apr 11, 2025
3538e4d
test: modified docstrings
Luiskitsu Apr 13, 2025
234b387
func: adding squeeze functionallity and extrapolation indices
Luiskitsu Apr 15, 2025
4c6d993
func: not returning extrapolation indices
Luiskitsu Apr 16, 2025
fcee820
test: made some coefficients smaller to pass tolerance
Luiskitsu Apr 18, 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/morphsqueeze.rst
View file
Open in desktop
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>
28 changes: 28 additions & 0 deletions src/diffpy/morph/morphs/morphsqueeze.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
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. The morphed
data is returned on the same grid as the unmorphed data.

Copy link
Contributor

Choose a reason for hiding this comment

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

it may be nice to have a usage example in the docstring. Basically copy-paste the code from the test to show how the morph can be instantiated and then used.

We normally put docstrings in the methods and in the constructor (the def __init__():). This class is inheriting the constructor from the base class, so I am not 100% sure how the docstring propagates through in the documentation, but without that, but I think if we put a docstring under the def morph(): which is overloading that method from the base class, it should become clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I was following the architecture of other morphs. They have a docstring bellow the class which is the main description and then another docstring under the function which are few words. Should I keep it consistent?
When I add an example in the docstring do I add it as code using >>>?

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have a group standard for this, but more recently I think we are leaning towards having the class-level docstring to be just a few words about the high level intent of the class and the docstring of the constructor to be have more detail. I did look into this question at one point and formed an opinion, but I don't fully remember what I found. It could have been when I was working on my beloved "DiffractionObject"s in diffpy.utils but I am not sure. We could look there. All documentation is better than lack of documentation, so the standards are just for us to figure out the most effective (user-useful) way to write the docs so that any time we spend on it has the most impact.

Configuration Variables
-----------------------
squeeze
Copy link
Contributor

Choose a reason for hiding this comment

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

More recently we have been using this syntax:

squeeze : list
  The polynomial coefficients [a0, a1, ..., an] for the squeeze function where the polynomial would be
  of the form a0 + a1 x + a2 x ^2 and so on.  The order of the polynomial is determined by the length
  of the list.

I removed the "array-like". Do we want to allow that or we just want a list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's just have a list here, I think makes sense and is consistent.

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)

return self.xyallout
60 changes: 60 additions & 0 deletions tests/test_morphsqueeze.py
View file
Open in desktop
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

squeeze_coeffs_list = [
# 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],
]
morph_target_grids = [
# UCs from issue 181: https://github.com/diffpy/diffpy.morph/issues/181
# UC2: Same grid
(np.linspace(0, 10, 101), np.linspace(0, 10, 101)),
# UC4: Target extends beyond morph
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add "same grid density" in the comment? likewise below on all the comments.

Do we have any where the grid density varies? I think we want tests for where the target is finer and when the morph is finer. We probably don't need to do the full matrix of longer/shorter/finer/coarser I think, so just for the finer/coarser tests they can be arrays over the same range.

We can discuss what we want the code to do in each of those cases. If one is numerically unstable or leads to inaccuracies we can choose to raise an exception and not allow users to do it as our desired behavior, but we should decide what we want to happen and have a test for that.

If we do raise an exception it goes in a separate test that we often give a name like test_function_bad() to differentiate it from test_function() and we check that if a condition is met (something coarser and we don't like that) then the right exception (and helpful error message) is raised.

We like error messages that are of the form "such and such went wrong. Try doing so and so to fix it", as this is most helpful to the user in general.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we have those cases covered. We have UC6 where the target is finer grid density and then UC8 where the morph is finer grid density. I'll update the comments in the test definitions to make this clearer, including specifying the grid density differences explicitly.

I think it's fine for the morph and target to have different grid densities in these tests, since we're not performing operations that compare the two arrays. That will become critical during refinement, where the grid densities will affect comparison between morph and target?.

(np.linspace(0, 10, 101), np.linspace(-2, 20, 221)),
# UC6: Target extends beyond morph; morph coarser
(np.linspace(0, 10, 101), np.linspace(-2, 20, 421)),
# UC8: Target extends beyond morph; target coarser
(np.linspace(0, 10, 401), np.linspace(-2, 20, 200)),
# UC10: morph starts earlier than target
(np.linspace(-2, 10, 121), np.linspace(0, 20, 201)),
# UC12: morph extends beyond target
(np.linspace(-2, 20, 221), np.linspace(0, 10, 101)),
]


@pytest.mark.parametrize("x_morph, x_target", morph_target_grids)
@pytest.mark.parametrize("squeeze_coeffs", squeeze_coeffs_list)
def test_morphsqueeze(x_morph, x_target, squeeze_coeffs):
y_target = np.sin(x_target)
squeeze_polynomial = Polynomial(squeeze_coeffs)
x_squeezed = x_morph + squeeze_polynomial(x_morph)
y_morph = np.sin(x_squeezed)
x_morph_expected = x_morph
y_morph_expected = np.sin(x_morph)
morph = MorphSqueeze()
morph.squeeze = squeeze_coeffs
x_morph_actual, y_morph_actual, x_target_actual, y_target_actual = morph(
x_morph, y_morph, x_target, y_target
)
assert np.allclose(y_morph_actual, y_morph_expected)
assert np.allclose(x_morph_actual, x_morph_expected)
assert np.allclose(x_target_actual, x_target)
assert np.allclose(y_target_actual, y_target)
Loading