Add Anderson's tests of NRM2 #925
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #925 +/- ##
========================================
Coverage 0.00% 0.00%
========================================
Files 1918 1918
Lines 188645 188757 +112
========================================
- Misses 188645 188757 +112
☔ View full report in Codecov by Sentry. |
|
Thanks @angsch. This is great and very impressive. Great to check a larger range of input. Impressive to improve test coverage from 41.7% to 91.7%. I do not have a strong opinion on either (a) compatibility with F2C and (b) testing INCX = 0. (a) WRT F2C, on the one hand, it bothers me to maintain compatibility with a software (F2C) that, I think, is not maintained anymore. On the other hand, we do not have to purposely go out of our ways to break compatibility. And it is true that testBLAS tests for NaNs and Infs. I'd rather be self-content (and break F2C), but we do not have to do this now. Note: One reason some NaNs and Infs checks were moved to testBLAS (as opposed to the BLAS test suite) was that the behavior for some BLAS subroutine was not defined in the presence of Infs and Nans, and so we were leery to move this to the BLAS test suite. But it feels like for NRM2, the expected behavior with NaNs and Infs is clear. (There were other reasons for testBLAS, namely, for example, a more modern test suite.) (b) WRT INCX = 0, it feels like if this is a valid input this should be tested. I think that testBLAS tests for INCX=0. So maybe this can wait too. |
|
Makes sense. I will fiddle in |
My only remaining concern is if the NaN check, currently inlined as appveyor couldn't find flang, so CI is stil ok. |
Thanks!
Right. Maybe we just add a comment in the TESTING then to explain that this can be improved. I think, for now, it is enough to leave at that. That is leave it at "inline a.ne.a" for checking for NaNs. Note that testing for NaNs in BLAS is also done by the project https://github.com/tlapack/testBLAS |
And we are trying to get the tests working with LAPACK's continuous testing framework, see #913. |
Add tests covering arrays with extreme numbers provided in Anderson E. (2017) Algorithm 978: Safe Scaling in the Level 1 BLAS ACM Trans Math Softw 44:1--28 https://doi.org/10.1145/3061665 [D,S]NRM2 coverage improves lines 59.1% -> 100.0% branches 30.0% -> 90.0%. [DZ,SC]NRM2 coverage improves lines 67.0% -> 100.0% branches 41.7% -> 91.7%
|
Based on my testing, everything is fine if the tests are compiled even with I consider this PR finalized, so please merge if you agree. |
|
Thanks @angsch! |
The existing tests of
NRM2cover only the branch where squaring does not require scaling. This commit adds Edward Anderson's tests in slightly modified form. His tests include arrays with extreme numbers such that the branches that require scaling are tested. The pass/fail threshold is the same as in Anderson's tests.The original tests are included in the archive provided alongside with
This commit modifies the tests such that they could be integrated into the existing BLAS-1 tests. It hardcodes the constants rather than computing them at compile time because some of the required intrinsics are not included in
f2c. The original idea was to make it easy for dependent projects to integrate the tests, either in this form or in C. However, I could not find a way to includeInfandNaNtests without breakingf2c, more below.In the current form, test coverage improves as follows.
Limitations:
The caseincx = 0is included in Anderson's original test, but is not included in this commit. Adding non-positive incx to the loop overincxintroduces compiler errors (computed indices in arrays < 1) in the existing tests. I have not looked into this.f2cbecause of the handling ofInfandNaN. It uses the workaround proposed by Anderson to avoid the compiler from complaining about overflow and division by zero whenInfandNaNare (intentionally) generated.HUGEis a function that is not permitted inf2c. If the tests ofInfandNaNare removed,f2cworks.Since
InfandNaNare covered by the TLAPACK tests, I would be happy to remove these two tests and keep the xBLAT1 testsf2cable. Further, is there any opinion about testingincx = 0? It could of course be added as a special case only forNRM2.