Skip to content

Commonize code between Fixed and Normed #151

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

Merged
merged 8 commits into from
Dec 30, 2019

Conversation

kimikage
Copy link
Collaborator

@kimikage kimikage commented Dec 23, 2019

This PR is the first part of #139.
I am very sorry about a conflict with #143, but in the long-term, I think this PR will help enhance the functions (e.g. checked arithmetic).

Because there are many changes, it is recommended that you track changes on a per-commit basis.

This PR includes a minor breaking change, i.e., this change the exception type of one(Q0f7) etc. from InexactError to ArgumentError.

I'm ready to fix isinteger (issue #120, PR #123) after the merging this PR. And then, I'll fix the issue #150.

Edit:
Rescheduling: this -> issue #153 -> isinteger -> rem -> issue #157

@kimikage kimikage force-pushed the commonize1 branch 3 times, most recently from a65286c to a2e3323 Compare December 28, 2019 13:36
Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

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

This is very nice, I strongly support this.

Are the methods removed in 2e19e67 really "useless" without a2e3323? (I haven't checked myself.)

src/normed.jl Outdated
xu = reinterpret(Unsigned, x)
k = Int(xu >> significand_bits(Tf))
k == 0 && return zero(N) # neglect subnormal numbers
significand = xu | (one(xu) << significand_bits(Tf))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
significand = xu | (one(xu) << significand_bits(Tf))
significand = xu | (oneunit(xu) << significand_bits(Tf))

(I failed to notice this earlier.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do we need to worry about the difference between one and oneunit for types without dimensions?
Anyway, I changed one to oneunit for future consistency.

Copy link
Member

Choose a reason for hiding this comment

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

Not so far, but in cases where you mean "1 of the given type" then oneunit is appropriate, no matter whether it differs from one.

one is a little weird because it could pretty much be defined as the constant 1.

@kimikage
Copy link
Collaborator Author

Are the methods removed in 2e19e67 really "useless" without a2e3323?

# comparison
==(x::T, y::T) where {T <: FixedPoint} = x.i == y.i
<(x::T, y::T) where {T <: FixedPoint} = x.i < y.i
<=(x::T, y::T) where {T <: FixedPoint} = x.i <= y.i

# Comparisons
<(x::T, y::T) where {T <: Normed} = reinterpret(x) < reinterpret(y)
<=(x::T, y::T) where {T <: Normed} = reinterpret(x) <= reinterpret(y)

They don't seem to differ in essence.
However, the tests of FixedPointNumbers.jl are somewhat poor, so we should to recheck the codes before release.

This changes the exception type of `one(Q0f7)` etc. from `InexactError` to `ArgumentError`.
@kimikage
Copy link
Collaborator Author

kimikage commented Dec 30, 2019

I added tests for reinterpret, although they have nothing to do with my changes directly.

Edit:
AppVeyor seems to be stuck.:sweat_smile:
AppVeyor seems to be stuck again on master branch.:cold_sweat:

I have confirmed that this has passed in my forked repository.
kimikage@e18c350
https://ci.appveyor.com/project/kimikage/fixedpointnumbers-jl/builds/29815705

@kimikage kimikage merged commit e18c350 into JuliaMath:master Dec 30, 2019
@kimikage kimikage deleted the commonize1 branch December 30, 2019 16:47
tlnagy added a commit to tlnagy/TiffImages.jl that referenced this pull request Jan 8, 2021
the minimum required FixedPointNumbers is 0.8.0 since it includes the
bswap code from
JuliaMath/FixedPointNumbers.jl#151
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