Increase coverage #2223
Increase coverage #2223
Conversation
Codecov ReportBase: 99.38% // Head: 99.63% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #2223 +/- ##
===========================================
+ Coverage 99.38% 99.63% +0.25%
===========================================
Files 359 361 +2
Lines 19821 20014 +193
===========================================
+ Hits 19699 19941 +242
+ Misses 122 73 -49
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
There are still some additional changes I want to do before merging |
| # with self.assertRaisesRegex(pybamm.ShapeError, "Cannot find shape"): | ||
| # pybamm.inner( | ||
| # pybamm.Matrix(np.zeros((2, 3))), pybamm.Matrix(np.ones((4, 5))) | ||
| # ).evaluate_ignoring_errors() |
There was a problem hiding this comment.
For some reason this test works on my computer but fails on GitHub actions. Locally it catches the pybamm.ShapeError but on GitHub it triggers the previous ValueError (e.g. see https://github.com/pybamm-team/PyBaMM/runs/7972858999?check_suite_focus=true). Any ideas?
There was a problem hiding this comment.
It could be something to do with whether debug_mode is True or False, it sometimes behaves differently if you run an individual test script vs the whole test suite
There was a problem hiding this comment.
It seemed to be related to tox. Anyway, when trying to debug I realised that the test wasn't actually triggering that error, but an error somewhere else, so not helping for the coverage. I also realised that that line actually threw an error that was not picked up earlier, so I changed that to a more generic statement like those in the previous lines. Let me know if that's OK or I should change it.
There was a problem hiding this comment.
Nvm. It did something very weird: when debugging it didn't access that line, but if I change it I get an error because pybamm.ShapeError is not raised.
|
I don't think I will have time to work on this in the next few weeks so I think it is better to merge. @tinosulzer, can you give it a quick look? |
| @@ -265,6 +267,19 @@ def test_inner(self): | |||
| # check doesn't evaluate on edges anymore | |||
| self.assertEqual(model.variables["inner"].evaluates_on_edges("primary"), False) | |||
|
|
|||
| # check _binary_jac | |||
There was a problem hiding this comment.
jacobian should be tested in test_jac.py
There was a problem hiding this comment.
I have added a test_jac_of_binary_operator in test_jac.py. Let me know if that is what you meant.
Various fixes to increase coverage and improve docstrings.