Address user warnings in tests #54
Conversation
…p.arrays specifically.
scico/test/linop/test_stack.py
Outdated
| B = Convolve(np.ones((2, 2)), (9, 15)) | ||
| a = jax.device_put(np.ones((3, 3))) | ||
| A = Convolve(a, (9, 15)) | ||
| b = jax.device_put(np.ones((2, 2))) |
There was a problem hiding this comment.
Can't just do A = Convolve(jax.device_put(np.ones((3, 3))), (9, 15))?
There was a problem hiding this comment.
Yeah, just thought it would be more readable to separate it, but I can def fix that and make it more compact.
There was a problem hiding this comment.
It is more readable, but the more compact form is already used in so many other places my leaning would be to just accept that as the standard way of doing this.
|
Test failures need to be resolved. I assume they're related to the changes in |
|
Yeah, the failing tests are due to the merge if im not mistaken so until that is fixed the PR has to remain on hold. |
In which file? Were there merge conflicts in |
bwohlberg
changed the title
…arnings for f.grad and fg.grad
….com:lanl/scico into FernandoDavis/address_user_warnings_in_tests
| # Ignores warning raised by ensure_on_device | ||
| warnings.filterwarnings(action="ignore", category=UserWarning) | ||
|
|
||
| # Verifies that there is a warning on f.grad and fg.grad |
There was a problem hiding this comment.
This addresses the TODO: should tweak test to verify that there is a warning on f.grad and fg.grad
| # Used to restore the warnings after the context is used | ||
| with warnings.catch_warnings(): | ||
| # Ignores warning raised by ensure_on_device | ||
| warnings.filterwarnings(action="ignore", category=UserWarning) |
There was a problem hiding this comment.
@lukepfister Hey Luke, whenever you have some time, is it necessary to make sure that the functionals are smooth for this test case? Or is it okay to suppress those user warnings and test that the warnings are properly being raised?
There was a problem hiding this comment.
I suppose that ideally the functionals would be smooth, but it's fine to just suppress-- it's not the point of this test.
There was a problem hiding this comment.
Alright then I'll have a chat with Brendt and the group about making any new functionals smooth, and if possible override this code snippet in the future to account for that.
There was a problem hiding this comment.
I suppose that ideally the functionals would be smooth, but it's fine to just suppress-- it's not the point of this test.
Do you mean that a better approach would be to modify the test so that it's only applied to functionals that are marked as smooth?
There was a problem hiding this comment.
That would be reasonable. If I remember correctly we don’t have very many smooth functional though.
Resolves #32 and #36.