Skip to content
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 inconsistencies and speed in C tests #11

Merged
merged 2 commits into from
Jun 4, 2024
Merged

Fix inconsistencies and speed in C tests #11

merged 2 commits into from
Jun 4, 2024

Conversation

mfasi
Copy link
Collaborator

@mfasi mfasi commented Jun 2, 2024

This change reduces the execution time of the tests by almost 50%. It ditches binary16 and TensorFloat-32 (which was only used in test 1a. anyway) and adds in E5M2.

I tried adding E4M3 instead, but tests 1c. and 2e. fail for that format.

* Delete unused macros
* Delete mostly unused types
* Replace format binary16 with E5M2
* Split overly large tests
* Fix duplicate test names
* Remove in-place tests that use same array as input
@mmikaitis
Copy link
Collaborator

Looks good. For formats with inf -> NaN on overflow we may need separate tests in the future.

@mfasi
Copy link
Collaborator Author

mfasi commented Jun 2, 2024

For formats with inf -> NaN on overflow we may need separate tests in the future.

I added tests for formats without infinities in 99665da. The tests extended case 2c.

However, if I choose unbalanced for E4M3, the tests mentioned above fail. I'm inclined to think we should address this before tagging a new release.

@mmikaitis
Copy link
Collaborator

I set up in cpfloat_test.ts

static size_t precision [] = {3, 8, 4};
static size_t emax [] = {15, 127, 8};
static size_t emin [] = {-14, -126, -6};

and 1c, 2e pass for me.

I have the following in 2c, however.

cpfloat_test(91092,0x1f58a0c00) malloc: *** error for object 0x14ff05aa0: pointer being freed was not allocated
cpfloat_test(91092,0x1f58a0c00) malloc: *** set a breakpoint in malloc_error_break to debug

@mfasi
Copy link
Collaborator Author

mfasi commented Jun 3, 2024

Are you changing the following line to

static size_t nformats = 3;

If not, the additional format will not be tested – and this might be what's causing the memory issue, although I'm not sure.

@mmikaitis
Copy link
Collaborator

in = 5.120000000000000e+02 [4647714815446351872]
out = inf [9218868437227405312]
ref = 5.120000000000000e+02 [4647714815446351872]

in = 4.960000000000000e+02 [4647433340469641216]
out = inf [9218868437227405312]
ref = 5.120000000000000e+02 [4647714815446351872]

The tests that fail seem to expect 512 as an answer, but the maximum representable value in E3M4 is 448. The reference is therefore calculated incorrectly. Furthermore, the output produces an infinity which E3M4 does not contain; a NaN should be produced.

@mfasi
Copy link
Collaborator Author

mfasi commented Jun 4, 2024

in = 5.120000000000000e+02 [4647714815446351872] out = inf [9218868437227405312] ref = 5.120000000000000e+02 [4647714815446351872]

in = 4.960000000000000e+02 [4647433340469641216] out = inf [9218868437227405312] ref = 5.120000000000000e+02 [4647714815446351872]

The tests that fail seem to expect 512 as an answer, but the maximum representable value in E3M4 is 448. The reference is therefore calculated incorrectly. Furthermore, the output produces an infinity which E3M4 does not contain; a NaN should be produced.

Thanks a lot for looking into this – I think you hit the nail on the head.

Currently, the C interface does not allow users to specify a format (such as E3M4), and what is being tested is a format with $e_\min = -6$, $e_\max = 8$, two signed infinities, and two signed zeros. Overflow behaviour is only tested in 2c and 2d, so I don't think the infinities are surprising.

The largest representable value, on the other hand, is calculated in the usual way as $2^{e_\max} (2 - 2^{1-p})$, which for this format is $2^8(2 - 2^{-3}) = 480$. As these tests only deal with the non-rounding cases, the input $512$ should not be there in the first place. Thus the problems are with the tests, and I think they boil down to these two lines:

https://github.com/north-numerical-computing/cpfloat/blob/9ae6e80138e44474070a76e47265a2dedada9c26/test/cpfloat_test.ts#L979C5-L979C66

https://github.com/north-numerical-computing/cpfloat/blob/9ae6e80138e44474070a76e47265a2dedada9c26/test/cpfloat_test.ts#L1695C1-L1695C76

I've just pushed a fix for these, and now all tests seem to pass for the format with unbalanced exponent.

@mmikaitis
Copy link
Collaborator

For E3M4 the maximum value has to be computed differently than usual, as 2^8(2-2^{-2})=448 instead of 2^8(2-2^{-3}). Check Table 2 in https://www.opencompute.org/documents/ocp-8-bit-floating-point-specification-ofp8-revision-1-0-2023-12-01-pdf-1. Are we not having any issues because of this?

@mfasi
Copy link
Collaborator Author

mfasi commented Jun 4, 2024

The C tests assume IEEE-compliant formats. It is true that E3M4 has a different maximum normal value, but there is no way to specify that in CPFloat at the moment, and the C interface work by default formats currently. These formats could be added to the C code, and this would probably simplify the MEX interface quite a bit. But we'd need to think somewhat carefully about the best way to do it.

@mfasi mfasi merged commit 0ddba87 into main Jun 4, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants