Skip to content
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

Remove cvxpy for weight minimization, fix test values and refactor tests codes #120

Merged
merged 14 commits into from
Oct 30, 2024
25 changes: 25 additions & 0 deletions news/cvxpy.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
**Added:**

* L-BFGS-B method in scipy for weight optimization in def get_weights
* Support for Python 3.13

**Changed:**

* <news item>

**Deprecated:**

* <news item>

**Removed:**

* cvxpy depenedency for linear weight optmization in def get_weights
* Support for Python 3.10

**Fixed:**

* Absolute tolerance for updated weighted matrix from 0.5 to 1e-05 in test_subroutines.py

**Security:**

* <news item>
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ maintainers = [
description = "A python package implementing the stretched NMF algorithm."
keywords = ['diffpy', 'PDF']
readme = "README.rst"
requires-python = ">=3.10, <3.13"
requires-python = ">=3.11, <3.14"
classifiers = [
'Development Status :: 4 - Beta',
'Environment :: Console',
Expand Down
1 change: 0 additions & 1 deletion requirements/conda.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
numpy
scipy
cvxpy
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As intended

diffpy.utils
numdifftools
1 change: 0 additions & 1 deletion requirements/pip.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
numpy
scipy
cvxpy
diffpy.utils
numdifftools
21 changes: 10 additions & 11 deletions src/diffpy/snmf/optimizers.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import cvxpy
import numpy as np
from scipy.optimize import minimize


def get_weights(stretched_component_gram_matrix, linear_coefficient, lower_bound, upper_bound):
Expand Down Expand Up @@ -36,20 +36,19 @@ def get_weights(stretched_component_gram_matrix, linear_coefficient, lower_bound
input set. Has length C

"""

stretched_component_gram_matrix = np.asarray(stretched_component_gram_matrix)
linear_coefficient = np.asarray(linear_coefficient)
upper_bound = np.asarray(upper_bound)
lower_bound = np.asarray(lower_bound)

problem_size = max(linear_coefficient.shape)
solution_variable = cvxpy.Variable(problem_size)

objective = cvxpy.Minimize(
linear_coefficient.T @ solution_variable
+ 0.5 * cvxpy.quad_form(solution_variable, stretched_component_gram_matrix)
)
constraints = [lower_bound <= solution_variable, solution_variable <= upper_bound]
# Set dynamic bounds based on the size of the linear coefficient
bounds = [(lower_bound, upper_bound) for _ in range(len(linear_coefficient))]
initial_guess = np.zeros_like(linear_coefficient)

cvxpy.Problem(objective, constraints).solve()
# Find optimal weights of linear coefficients
def obj_func(y):
return linear_coefficient.T @ y + 0.5 * y.T @ stretched_component_gram_matrix @ y

return solution_variable.value
result = minimize(obj_func, initial_guess, method="L-BFGS-B", bounds=bounds)
return result.x
13 changes: 9 additions & 4 deletions tests/test_optimizers.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@

from diffpy.snmf.optimizers import get_weights

tm = [
([[[1, 0], [0, 1]], [1, 1], [0, 0], [1, 1]], [0, 0]),
test_matrix = [
# ([stretched_component_gram_matrix, linear_coefficient, lower_bound, upper_bound], expected)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added an in-line comment to see the order of the input data in the test matrix

([[[1, 0], [0, 1]], [1, 1], 0, 0], [0, 0]),
Copy link
Contributor Author

@bobleesj bobleesj Oct 26, 2024

Choose a reason for hiding this comment

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

Fixing syntax bug -

fixing from [0,0] to to 0, 0]

0,0] refers to lower_bound and upper_bound, respectively.

([[[1, 0], [0, 1]], [1, 1], -1, 1], [-1, -1]),
([[[1.75, 0], [0, 1.5]], [1, 1.2], -1, 1], [-0.571428571428571, -0.8]),
([[[0.75, 0.2], [0.2, 0.75]], [-0.1, -0.2], -1, 1], [0.066985645933014, 0.248803827751196]),
Expand All @@ -13,8 +14,12 @@
]


@pytest.mark.parametrize("tm", tm)
@pytest.mark.parametrize("tm", test_matrix)
def test_get_weights(tm):
stretched_component_gram_matrix = tm[0][0]
linear_coefficient = tm[0][1]
lower_bound = tm[0][2]
upper_bound = tm[0][3]
expected = tm[1]
actual = get_weights(tm[0][0], tm[0][1], tm[0][2], tm[0][3])
actual = get_weights(stretched_component_gram_matrix, linear_coefficient, lower_bound, upper_bound)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pass variables instead of tm[0][0], etc. which is much harder to read for future generations.

assert actual == pytest.approx(expected, rel=1e-4, abs=1e-6)
13 changes: 8 additions & 5 deletions tests/test_subroutines.py
Copy link
Contributor Author

@bobleesj bobleesj Oct 26, 2024

Choose a reason for hiding this comment

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

Problem- the previous code in line 212 in this file had an absolute tolerance of 0.5!

np.testing.assert_allclose(actual, expected, rtol=1e-03, atol=0.5)

When I modified atol to 1e-05

np.testing.assert_allclose(actual, expected, rtol=1e-03, atol=1e-05)

the original code started to fail (3-4 out of 7-8 test cases failed).

Since our new scipy minimization function has been implemented correctly per our tests above, I've updated the numbers for the cases that are failing - CI is passing as well with the new implmentation.

Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ def test_get_stretched_component(tgso):
[[0.78, 0.12], [0.5, 0.5]],
None,
],
[[0, 1], [1, 1]],
[[0.533333, 0.933333], [0.533333, 0.933333]],
),
(
[2, 3, [[0.5], [0.5]], [[1, 2.5], [1.5, 3], [2, 3.5]], [[1, 2], [3, 4], [5, 6]], 1, [[0.5], [0.5]], None],
Expand Down Expand Up @@ -155,7 +155,7 @@ def test_get_stretched_component(tgso):
[[0.9, 0.4, 0.5], [1, 0, 0.4], [0, 0, 0.98]],
None,
],
[[1, 0.0651, 0], [0.5848, 0.0381, 0.1857], [0, 1, 1]],
[[1.0, 0.0900485, 0.0], [0.585632, 0.497497, 0.179719], [0.0, 0.52223655, 1.0]],
),
([2, 2, [[0.5], [0.5]], [[0, 0], [0, 0]], [[0, 0], [0, 0]], 1, [[0.6], [0.4]], "align"], [[0], [0]]),
([1, 3, [[0.5, 0.3]], [[1], [1.1], [1.3]], [[1, 2], [2, 3], [3, 2]], 2, [[0.6, 0.4]], None], [[1, 1]]),
Expand All @@ -170,7 +170,7 @@ def test_get_stretched_component(tgso):
[[0.78, 0.12], [0.5, 0.5]],
"align",
],
[[0, 0], [1.0466, 1.46]],
[[0, 0], [0.8, 1.4]],
),
(
[
Expand All @@ -196,7 +196,7 @@ def test_get_stretched_component(tgso):
[[0.9, 0.4, 0.5], [1, 0, 0.4], [0, 0, 0.98]],
"align",
],
[[1.2605, 0.0552, 0], [0.2723, 0, 0], [0, 1.0538, 1.1696]],
[[1.281265, 0.104355, 0], [0.0, 0.0, 0.0], [0.239578, 0.965215, 1.162571]],
),
([2, 2, [[0.5], [0.5]], [[0, 0], [0, 0]], [[0, 0], [0, 0]], 1, [[0.6], [0.4]], "align"], [[0], [0]]),
([1, 3, [[0.5, 0.3]], [[1], [1.1], [1.3]], [[1, 2], [2, 3], [3, 2]], 2, [[0.6, 0.4]], "align"], [[1.3383, 2]]),
Expand All @@ -208,8 +208,11 @@ def test_update_weights_matrix(tuwm):
actual = update_weights_matrix(
tuwm[0][0], tuwm[0][1], tuwm[0][2], tuwm[0][3], tuwm[0][4], tuwm[0][5], tuwm[0][6], tuwm[0][7]
)

print("Actual test get stretched component", actual)
print("Expected test get stretched component", tuwm[1])
expected = tuwm[1]
np.testing.assert_allclose(actual, expected, rtol=1e-03, atol=0.5)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

atol=0.5 is way too high considering we are comparing numbers between 0 and 1.2, etc.

np.testing.assert_allclose(actual, expected, rtol=1e-03, atol=1e-05)


tgrm = [
Expand Down