-
Notifications
You must be signed in to change notification settings - Fork 27
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
Fix check in unsymmetric scaling test #211
Conversation
cnt counts column-wise, so should be checked up to the number of columns a%n, not the number of rows a%m. Noticed when doing tests for ralna#207 by allocating the exact sizes for each randomly generated matrix rather than the oversized maximum sizes.
@mjacobse yes agreed. It it a mystery why the oversized maximum sizes are allocated in the tests! |
To be fair it simplifies things by doing the allocation just once and reusing it for all test cases. But it really has the potential to hide bugs. Would you like me to open a pull request with specifically reallocated arrays for each test case? |
Yes please, that would be amazing :) |
rinf is the inf norm of all scaled rows, so has length a%m not a%n. Basically same as ralna#211
rinf is the inf norm of all scaled rows, so has length a%m not a%n. Basically same as #211
Arrays for testing the scaling functions with random matrices were only allocated once with the maximum dimensions of all possibly generated matrices. That makes it simple, but unfortunately also has the potential to hide possible bugs, as seen in ralna#211 and ralna#213. To improve the chance to detect such bugs, this reallocates the arrays to the exact dimensions for each current test case.
Arrays for testing the scaling functions with random matrices were only allocated once with the maximum dimensions of all possibly generated matrices. That makes it simple, but unfortunately also has the potential to hide possible bugs, as seen in #211 and #213. To improve the chance to detect such bugs, this reallocates the arrays to the exact dimensions for each current test case.
cnt
counts column-wise, so should be checked up to the number of columnsa%n
, not the number of rowsa%m
. Noticed when doing tests for #207 by allocating the exact sizes for each randomly generated matrix rather than the oversized maximum sizes. Perhaps that would be a good general improvement for the tests, even with #207 already fixed?