Skip to content

Modify Fixed constructors #170

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 1 commit into from
Feb 2, 2020
Merged

Conversation

kimikage
Copy link
Collaborator

This adds explicit input range checks. As a result, the constructors throw ArgumentError instead of InexactError for out-of-range inputs.

@codecov-io
Copy link

Codecov Report

Merging #170 into master will increase coverage by 0.36%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #170      +/-   ##
==========================================
+ Coverage   88.32%   88.68%   +0.36%     
==========================================
  Files           5        5              
  Lines         377      389      +12     
==========================================
+ Hits          333      345      +12     
  Misses         44       44
Impacted Files Coverage Δ
src/fixed.jl 89.01% <100%> (+1.66%) ⬆️

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 cb29336...f3f963a. Read the comment docs.

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.

If other packages create new AbstractFloat subtypes the big method will be used by default. I was initially a bit skeptical but upon reflection it seems like a slow-but-safe choice.

@kimikage
Copy link
Collaborator Author

kimikage commented Feb 2, 2020

Thank you for your review.

I think there are roughly two types of user-defined float types. One is a thin wrapper like struct MyFloat<:AbstractFloat; v::Float64; end and the other is a more complicated type with special functionality.
For the former, the current code requires the implementation of trunc and rem, and promotion rules with integers. That results in unnecessary implementation costs and execution costs. Therefore, the end users or package developers should implement conversion code using its inner type.

I initially thought of using floattype(F) when promote_type(floattype(F), typeof(x)) == floattype(F). Perhaps it can provide a faster-and-safe way than the big method. However, I think it is a "premature optimization". The Float32/Float64 <--> Integer conversions are not slow but not fast, and above all, those conversion methods can throw exceptions.

If we actually find that it is better to use floattype(F), then, we should use it.

This adds explicit input range checks.
@kimikage
Copy link
Collaborator Author

kimikage commented Feb 2, 2020

The speedup can be done after releasing v0.8.0, so I will merge this first. With this PR, I am ready to release v0.8.0.

@kimikage kimikage merged commit 03057d4 into JuliaMath:master Feb 2, 2020
@kimikage kimikage deleted the ctor_fixed branch February 2, 2020 17:18
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.

3 participants