Refactor tests #251

ewu63 · 2021-05-07T15:22:39Z

Purpose

This PR refactors the existing tests by introducing a test class that handles various shared tasks

run an optimization
check that solutions are close
check that the final inform is correct
check the history file contains expected contents
check hot start

I also added parameterized tests etc. for efficiency.

Not all existing tests use this class however:

the two SNOPT test files can probably use this if we want, but will require a bit more work
for some reason NSGA2 will segfault if the objfunc is defined within the class. Absolutely no idea why..

I'll keep this as a draft PR for now. The main thing I want to check is that we're not losing any coverage through this, so if reviewers can also check against the old tests and see if I missed anything. Finally, the way the tests are structured can probably improved, so any suggestions are welcome.

Type of change

What types of change is it?
Select the appropriate type(s) that describe this PR

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (non-backwards-compatible fix or feature)
Code style update (formatting, renaming)
Refactoring (no functional changes, no API changes)
Documentation update
Maintenance update
Other (please describe)

codecov · 2021-05-07T15:28:45Z

Codecov Report

Merging #251 (42b5144) into master (b955064) will decrease coverage by 0.53%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #251      +/-   ##
==========================================
- Coverage   83.97%   83.44%   -0.54%     
==========================================
  Files          22       22              
  Lines        3252     3250       -2     
==========================================
- Hits         2731     2712      -19     
- Misses        521      538      +17

Impacted Files	Coverage Δ
pyoptsparse/pyoptsparse/pyOpt_optimization.py	`78.83% <0.00%> (-1.50%)`	⬇️
pyoptsparse/pyoptsparse/pyOpt_history.py	`81.70% <0.00%> (-0.82%)`	⬇️
pyoptsparse/pyoptsparse/pyOpt_optimizer.py	`84.25% <0.00%> (-0.70%)`	⬇️
pyoptsparse/pyoptsparse/pyALPSO/pyALPSO.py	`85.18% <0.00%> (-0.36%)`	⬇️
pyoptsparse/pyoptsparse/pySNOPT/pySNOPT.py	`90.09% <0.00%> (-0.34%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b955064...42b5144. Read the comment docs.

sseraj

This is a major improvement! I don't see any obvious reduction in coverage other than SNOPT versions prior to 7.7.7. Is that intentional?

sseraj · 2021-05-12T02:39:31Z

test/test_hs015.py

-        assert_allclose(sol_xvars, dv["xvars"], atol=tol, rtol=tol)
-        # we check either optimum via try/except
-        try:
-            if optName == "SNOPT" and opt.version != "7.7.7":


The stable image is failing because these checks have been removed.

Good point, will add

sseraj · 2021-05-12T02:42:52Z

test/test_hs015.py

-        self.optimize_with_hotstart("PSQP", 5e-12, optOptions=optOptions)
+        # dv = sol.getDVs()
+        # sol_xvars = [sol.variables["xvars"][i].value for i in range(2)]
+        # assert_allclose(sol_xvars, dv["xvars"], atol=tol, rtol=tol)


Are these commented lines supposed to be here?

Ah, right these were testing sol.variables etc. I will try to generalize these and make sure the Solution object contained the right values in different attributes.

I added some tests, but not all since it's a bit harder to generalize. I've opened #256 to address this, among other things.

test/test_large_snopt.py

marcomangano

Altright, I think I listed all the remaining comments I had. Note that some of them are not necessarily related to your edits, but additional comments/info we could add during this refactoring.

This is a substantial PR and some commits are pretty big and hard to follow, so for future PRs I would suggest to either break them further or, more in general, add a comment to the commit message to clarify the rationale behind it?

test/test_hs071.py

test/testing_utils.py

test/test_hs071.py

test/test_tp109.py

marcomangano · 2021-05-13T21:52:29Z

test/test_tp109.py


        # Check Solution
-        assert_allclose(sol2.objectives["obj"].value, 0.536206927538e04, atol=1e-2, rtol=1e-2)
+        self.assert_solution(sol2, 1e-2)


See above - Why is this tolerance so large?

Hmm good point, I think the idea is that this is loose because SNOPT may not be available, so then we just assert the ALPSO answer. I will fix this.

Actually ALPSO seems...kinda broken. I'll open an issue but I prefer not fixing this in this PR.

marcomangano · 2021-05-13T21:57:48Z

test/testing_utils.py

+
+
+def get_dict_distance(d, d2):
+    """This converts a flat 1D dict to array using sorted keys, then computes the L2 distance"""


took a while to understand why this was necessary, nice trick, but are we sure it is the most robust approach?

Not sure what you mean here. The code is not 100% robust because the numpy array casts aren't 100% fool proof, but the approach itself should be valid no?

Yeah I was just thinking about cases where the L2 norm would not be the the best approach but I think for testing specifically this is not an issue. Good to go!

test/testing_utils.py

sseraj

This looks good to me other than one thing

test/testing_utils.py

ewu63 · 2021-05-14T14:50:33Z

This is a substantial PR and some commits are pretty big and hard to follow, so for future PRs I would suggest to either break them further or, more in general, add a comment to the commit message to clarify the rationale behind it?

Yeah sorry about that, I tried to break it up but as you can see, it's hard to do when I'm changing many things at once. Because I will be testing things as I develop, it's hard to keep a nice linear git history as well. I will try to make my commit messages better though, I don't think I was very good at that.

Anyways, I think I addressed most of the concerns. Please take another look and let me know.

ewu63 added 17 commits April 15, 2021 18:41

added test util file

9a5f851

refactored into a separate testing class

6040015

renamed check to assert

d112e2d

simplified some code

79b82df

updated test_sparse

6a04f70

renamed utils to testing_utils

eb78d02

moved optimize_with_hotstart and check_hist_file to test class

8aceb38

updated tp109

a69ce72

changed sol.fStar to a dictionary format

e7b4a33

add support for multiple solutions in asserts

88e2730

updated hs015

7846109

generalized testing functions

631dcb4

Merge remote-tracking branch 'mdolab/master' into update-tests

83bac91

changed obj back to scalar instead of dict

8477cf6

updated base class API

4748aba

only skip tests if non built-in optimizer

9ebe1ba

updated isort

f23408e

ewu63 requested review from marcomangano and sseraj May 7, 2021 15:22

ewu63 and others added 4 commits May 7, 2021 12:50

Update .flake8

02e255d

Merge branch 'master' into update-tests

c450737

added isort.cfg

f964de6

Merge remote-tracking branch 'mdolab/update-tests' into update-tests

717de08

ewu63 mentioned this pull request May 8, 2021

IPOPT duplicated history handling #252

Merged

12 tasks

sseraj reviewed May 12, 2021

View reviewed changes

ewu63 added 4 commits May 12, 2021 14:38

moved test_large_snopt to test_large_sparse

8070e95

switched to parameterized tests

84bd360

made sparse tests smaller

6f1da82

added version check for SNOPT < 7.7.7

c3248d7

ewu63 added 2 commits May 13, 2021 13:12

updated NSGA2 test

86f327e

stopped storing opt in testing class, was causing obscure pickle error

804cae5

ewu63 marked this pull request as ready for review May 13, 2021 17:35

ewu63 requested a review from a team as a code owner May 13, 2021 17:35

isort

3117dfe

ewu63 linked an issue May 13, 2021 that may be closed by this pull request

Update testing framework #145

Closed

ewu63 mentioned this pull request May 13, 2021

Update testing framework #145

Closed

removed unnecessary code from ALPSO wrapper

e627e58

ewu63 requested a review from sseraj May 13, 2021 18:55

marcomangano reviewed May 13, 2021

View reviewed changes

sseraj reviewed May 13, 2021

View reviewed changes

test/testing_utils.py Outdated Show resolved Hide resolved

ewu63 added 4 commits May 14, 2021 10:02

switched from packaging to pkg_resources

85dbda1

switched to using self.id() to access the test name

1f436cf

removed duplicate code

c5da3a9

added some comments

42b5144

sseraj approved these changes May 14, 2021

View reviewed changes

ewu63 requested a review from marcomangano May 15, 2021 18:46

marcomangano approved these changes May 15, 2021

View reviewed changes

marcomangano merged commit bf140e2 into master May 15, 2021

marcomangano deleted the update-tests branch May 15, 2021 22:26

marcomangano mentioned this pull request Aug 30, 2021

Improve skipTest handling #177

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor tests #251

Refactor tests #251

ewu63 commented May 7, 2021

codecov bot commented May 7, 2021 •

edited

sseraj left a comment

sseraj May 12, 2021

ewu63 May 12, 2021

sseraj May 12, 2021

ewu63 May 12, 2021

ewu63 May 13, 2021

marcomangano left a comment

marcomangano May 13, 2021

ewu63 May 14, 2021

ewu63 May 14, 2021

marcomangano May 13, 2021

ewu63 May 14, 2021

marcomangano May 15, 2021

sseraj left a comment

ewu63 commented May 14, 2021 •

edited



		def get_dict_distance(d, d2):
		"""This converts a flat 1D dict to array using sorted keys, then computes the L2 distance"""

Refactor tests #251

Refactor tests #251

Conversation

ewu63 commented May 7, 2021

Purpose

Type of change

codecov bot commented May 7, 2021 • edited

Codecov Report

sseraj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcomangano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sseraj left a comment

Choose a reason for hiding this comment

ewu63 commented May 14, 2021 • edited

codecov bot commented May 7, 2021 •

edited

ewu63 commented May 14, 2021 •

edited