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

Remove emin=1-emax restriction and specify emin explicitly #7

Merged
merged 26 commits into from
May 29, 2024
Merged

Conversation

mmikaitis
Copy link
Collaborator

At present a test in ctest is failing, while matlab/octave tests pass.

mfasi and others added 23 commits April 30, 2024 11:28
Specifically, e4m3 in the OCP standard does not represent infinities to expand the maximum exponent to 8.
This allows to make fp8-e4m3 fully OCP compliant, where it is specified with emax = 8 and emin = -6.
As per https://stackoverflow.com/questions/17534840/sed-throws-bad-flag-in-substitute-command.

I hope this does not break sed -i use on other operating systems. I will revert the patch if it does and a separate logic will be needed to invoke sed -i differently on macOS.
@mmikaitis mmikaitis requested a review from mfasi May 8, 2024 15:39
@mmikaitis mmikaitis changed the title Remove emin=1-emin restriction and specify emin explicitly Remove emin=1-emax restriction and specify emin explicitly May 8, 2024
Copy link
Collaborator

@mfasi mfasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Mantas – this is an impressive PR!

I think it's essentially ready to be merged into main – I have just a few minor comments.

My main question is whether the order of $e_\min$ and $e_\max$ makes a difference. Currently, we have $e_\max$ first, and this is done consistently in documentation and code.

In books and documentation, $e_\min$ often comes first, possibly because $e_\min \le e_\max$. One notable exception is the IEEE standard, but this makes sense to me: there $e_\min$ is defined in terms of $e_\max$, so this is the only correct order.

Should we swap them or keep them as they are?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated

The library was originally intended as a faster version of the MATLAB function `chop` [[2]](#ref2), which is [available on GitHub](https://github.com/higham/chop).
The latest versions of the library have a variety of subtle differences compared with `chop`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should state explicitly the version of the two packages where things started to diverge?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a list of items. Feel free to add further items or modify.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
mex/cpfloat.m Outdated Show resolved Hide resolved
mex/cpfloat.m Outdated
% function CHOP available at https://github.com/higham/chop.
% The interface of CPFLOAT is partly compatible with that of the MATLAB
% function CHOP available at https://github.com/higham/chop. The main
% difference is that CPFLOAT requires EMIN specified in FPOPTS.params.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few other differences though: the random number generator can be passed to chop, the OCP formats are (will be) different, etc. Do we want to specify what differences are there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now linking to the list of differences in README.md

Copy link
Collaborator

@mfasi mfasi May 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I think this is true for Windows. Were you able to compile the PCG on a Mac and link it to the MEX interface? If yes, then I'll explicitly mention that this is true for Windows only; otherwise, I'll say it's true unless you're on Linux.

mex/cpfloat_compile.m Show resolved Hide resolved
src/cpfloat_docmacros.h Outdated Show resolved Hide resolved
src/cpfloat_docmacros.h Show resolved Hide resolved
@mmikaitis mmikaitis merged commit 653d1eb into main May 29, 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