Add sparse matrix and scaling tests#486
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #486 +/- ##
===========================================
- Coverage 82.99% 54.22% -28.78%
===========================================
Files 1 1
Lines 147 225 +78
===========================================
Hits 122 122
- Misses 25 103 +78 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
marcomangano
left a comment
marcomangano
left a comment
There was a problem hiding this comment.
Thanks a lot for doing this! Just a few very minor points below
| assert_sens_matches_analytic(funcsSens, atol=atol) | ||
|
|
||
| # test that we get real derivs for cs | ||
| if sensType == "cs": |
There was a problem hiding this comment.
This is interesting, I would have thought that the assert_allclose would have caught a type mismatch. Would it make sense to move this before the call to assert_sens_matches_analytic?
There was a problem hiding this comment.
strict=True would assert dtype also but defaults to False. Typically we want it to broadcast and ignore shape but not dtype.
| return funcs, False | ||
|
|
||
|
|
||
| def build_optProb(objfun=objfunc, xScale=1.0, conScale=1.0): |
There was a problem hiding this comment.
Minor curiosity: Is the mismatch between arg and function name intentional? Naming best practice?
There was a problem hiding this comment.
A little bit? objfunc is what we use to call functions, but Optimization object's field is objfun. I can harmonize this.
There was a problem hiding this comment.
Oh no I remember now, I didn't want to shadow a function with a variable with the same name.
| for dvGroup in ANALYTIC[funcKey]: | ||
| self.assertFalse(np.iscomplexobj(funcsSens[funcKey][dvGroup])) | ||
|
|
||
| def test_failed_eval(self): |
There was a problem hiding this comment.
For sake of future maintainability, do you mind adding a couple of lines of comment to describe these last two tests?
The first one could be "Testing that failed flags from objfunc are caught by the Gradient class", and TBH I am confused about what we are testing in the last one. Is that to ensure that the original gradient object is not scaled, similar to l.350 in test_scaling.py?
There was a problem hiding this comment.
Yes the difference here is using CS, rather than analytic derivatives. This is more integration test while unit tests for the mapping funcs are below.
| jac = convertToCSR(dense) | ||
|
|
||
| # _mapConJactoOpt works in place: J_opt = diag(conScale) . J . diag(invXScale) | ||
| self.optProb._mapConJactoOpt(jac) |
There was a problem hiding this comment.
This is well outside the scope of the PR, but I wonder what is the rationale for _mapObjGradtoOpt returns an object (operating on a copy of the original gobj), while _mapConJactoOpt?
There was a problem hiding this comment.
It's a bit of an implementation detail. _mapObjGradtoOpt operates on a numpy array directly, while _mapConJactoOpt is from older code that also operates on sparse matrices, and for efficiency it mutates in place. This is something we should probably unify, and i think some of the type hints here are wrong.
| def test_from_csc(self): | ||
| coo = convertToCOO(_CSC) | ||
| rows, cols, data = coo["coo"] | ||
| assert_array_equal(rows, [0, 2, 1, 0, 2]) |
There was a problem hiding this comment.
These are hardcoded and different from _COO because the "reading order" changes based on the initial format, right? Could you add a comment line similarly to what you did in the following tests?
2 participants
Purpose
Add a few tests, partially addresses #256.
Expected time until merged
A few days
Type of change
Testing
Checklist
ruff checkandruff formatto make sure the Python code adheres to PEP-8 and is consistently formattedfprettifyor C/C++ code withclang-formatas applicable