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

Add checked_add and checked_sub #152

Closed
kimikage opened this issue Dec 24, 2019 · 8 comments · Fixed by #190
Closed

Add checked_add and checked_sub #152

kimikage opened this issue Dec 24, 2019 · 8 comments · Fixed by #190
Milestone

Comments

@kimikage
Copy link
Collaborator

kimikage commented Dec 24, 2019

As the first measure against #41, I want to add checked_add and checked_sub (and checked_neg, checked_abs) so that we can try CheckedArithmetic.

I think it is better to support checked_mul at the same time as or after the optimization of current multiplication implements. Adding checked_mul implies introducing unchecked mul, i.e. breaking changes.

@kimikage
Copy link
Collaborator Author

BTW, if checked arithmetic becomes the default and the overflow-safe promotion rules are added in the future, I think that using the @fastmath macro to access the successor methods to the current unsafe arithmetic might be a good idea. However, I cannot compromise any more than that.

@kimikage
Copy link
Collaborator Author

BTW, the integer types of Rust have saturating_add(), wrapping_sub() and so on, and their default operators check overflows.

The removal of @fastmath (changing to more specific methods) was proposed (cf. JuliaLang/julia#36246). I think adding add_fast and so on for FixedPoint seems to be a bad idea, now.

One option is to add the @saturating and @wrapping macros to CheckedArithmetic (or another new package). I think the new macros also solves the problem that @checked is too exhaustive.

@kimikage
Copy link
Collaborator Author

Also, in the future, the default arithmetic may be changed to the checked arithmetic.

What's your plan for eliminating the performance hit?

Originally posted by @timholy in #209 (comment)

It is to give up.:sweat_smile: In any case, we haven't reached the stage of deciding that yet. PR #190 is just a small step.

Originally posted by @kimikage in #209 (comment)

However, I am interested in Cassette.jl. I think the injection is useful in end user codes. If checked arithmetic is needed only for development/debugging and not for the final code, I think that the default arithmetic can be the current wrapping arithmetic.

It might be also a good idea for CheckedArithmetic.jl to use Cassette.jl. However, in that case, it is better to separate the interface and implementation into separate packages. As you know, I have concerns about the accumulatortype. (cf. #146 (comment))

In the future (Julia v2.0?), I hope that handling wrapping/saturating/checked arithmetic switching should be supported by Base or Core. I would like to do the case study for that within FixedPointNumbers. If the case study goes well, it will be the motivation to support saturated arithmetic functions (and the intrinsic functions for LLVM) in Base as of Julia v1.x.

@timholy
Copy link
Member

timholy commented Aug 13, 2020

Cassette is cool technology but beware of the compile-time latency. Conceptually it's not very different from

img1checked, img2checked = reinterpret(N0f8Checked, img), reinterpret(N0f8Checked, img)
imgdiff = img1checked-img2checked

which would be a more "surgical" approach to checked-aware recompilation (though perhaps less easy to automate).

While catching issues during development is of course a good idea, don't forget the interactive convenience of visualizing img1 - img2, and the head-scratching that can result when you have wrapping or saturating arithmetic. Checked arithmetic is also quite annoying in such circumstances, though at least it might give you an inkling of what's to blame. float.(img1) - float.(img2) is an easy way to fix problems but not all users will think of it.

@kimikage kimikage reopened this Aug 13, 2020
@kimikage
Copy link
Collaborator Author

kimikage commented Aug 13, 2020

The original purpose of this issue was achieved by PR #190, but I reopened this just for discussion.

@timholy
Copy link
Member

timholy commented Aug 13, 2020

Oh, I think you could leave this closed if you want. As you say, you fixed it! 🎉 Having checked arithmetic is great, I'm just pointing out that it doesn't solve all the problems that I think we might want to address.

@kimikage
Copy link
Collaborator Author

It may be better to open the topic in the Discourse or Zulip. 😄

@kimikage
Copy link
Collaborator Author

I will close this issue by moving the discussion place in this repository to issue #227.

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 a pull request may close this issue.

2 participants