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

Change the default arithmetic of * for Normed to wrapping #236

Merged
merged 1 commit into from
Nov 6, 2020

Conversation

kimikage
Copy link
Collaborator

@kimikage kimikage commented Oct 29, 2020

The implementation of multiplication for Normed was changed in PR #213, but its arithmetic defaulted to checked arithmetic for backward compatibility.

Although the discussion in Discourse is not aimed solely at building specific consensus, but at least, there is no objection to changing the default arithmetic of multiplication for Normed to wrapping arithmetic (i.e. Option 2.).

So this PR changes the default arithmetic to wrapping. This increases the risk of not noticing the overflow, but improves the consistency of the arithmetic rules.

julia> 1N0f8 * 1N0f8 # N0f8, N0f16, N0f32 and N0f64 are not affected by this change because they do not cause overflow
1.0N0f8

julia> 3N4f12 * 7N4f12 # v0.8
ERROR: ArgumentError: Normed{UInt16,12} is a 16-bit type representing 65536 values from 0.0 to 16.0037; cannot represent 21.0

julia> 3N4f12 * 7N4f12 # w/o this PR
ERROR: OverflowError: 3.0N4f12 * 7.0N4f12 overflowed for type N4f12

julia> 3N4f12 * 7N4f12 # with this PR
4.9961N4f12

You can still use checked arithmetic explicitly.

julia> using Base.Checked

julia> checked_mul(3N4f12, 7N4f12)
ERROR: OverflowError: 3.0N4f12 * 7.0N4f12 overflowed for type N4f12

@kimikage kimikage added the breaking Major breaking change label Oct 29, 2020
@kimikage kimikage mentioned this pull request Oct 29, 2020
@codecov
Copy link

codecov bot commented Oct 29, 2020

Codecov Report

Merging #236 into master will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #236      +/-   ##
==========================================
- Coverage   96.47%   96.46%   -0.01%     
==========================================
  Files           6        6              
  Lines         737      736       -1     
==========================================
- Hits          711      710       -1     
  Misses         26       26              
Impacted Files Coverage Δ
src/normed.jl 95.90% <ø> (-0.02%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6236e66...0530447. Read the comment docs.

@kimikage
Copy link
Collaborator Author

kimikage commented Nov 6, 2020

I will do a final confirmation before releasing v0.9.

@kimikage kimikage merged commit 1e14ce6 into JuliaMath:master Nov 6, 2020
@kimikage kimikage deleted the default_normed_mul branch November 6, 2020 23:54
@kimikage kimikage mentioned this pull request Apr 30, 2024
38 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Major breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant