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

Support floattype(Rational) and require <:AbstractFloat for fallback #177

Merged
merged 3 commits into from
Jun 21, 2020

Conversation

timholy
Copy link
Member

@timholy timholy commented Mar 17, 2020

The more "dangerous" (breaking) part of this is that it seems to be a non-sequitur to allow floattype to fall back to returning a type that is not an AbstractFloat. Consequently this will now return a MethodError under more circumstances.

@timholy timholy force-pushed the teh/floattype_rational branch 2 times, most recently from afe3f9b to 186aa63 Compare March 18, 2020 10:50
@timholy timholy changed the title Support floattype(Rational) and guarantee AbstractFloat return Support floattype(Rational) and require <:AbstractFloat for fallback Mar 18, 2020
@timholy
Copy link
Member Author

timholy commented Mar 18, 2020

OK, this has been pretty extensively reworked. First, this issues a deprecation warning rather than an error. Second, it clarifies when it's OK to extend this to support non-AbstractFloats.

I'm contemplating releasing this as part of the 0.8 series, rather than as the beginning of a 0.9 series. I've traditionally viewed depwarns as more breaking than others have, but I'm beginning to be more cautious about tagging-churn in the dependencies. Thoughts?

@codecov-io
Copy link

codecov-io commented Mar 18, 2020

Codecov Report

Merging #177 into master will decrease coverage by 0.23%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #177      +/-   ##
==========================================
- Coverage   87.84%   87.61%   -0.24%     
==========================================
  Files           5        6       +1     
  Lines         436      444       +8     
==========================================
+ Hits          383      389       +6     
- Misses         53       55       +2     
Impacted Files Coverage Δ
src/deprecations.jl 0.00% <0.00%> (ø)
src/FixedPointNumbers.jl 83.50% <100.00%> (+1.98%) ⬆️

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 0872417...7eb7513. Read the comment docs.

@timholy
Copy link
Member Author

timholy commented Mar 18, 2020

A demo in the OP of JuliaGraphics/ColorTypes.jl#177 needed to add support for floattype(::Type{<:Complex}). I am not certain whether that would be worth adding here, but if so this would be a good PR for it.

@kimikage
Copy link
Collaborator

It seems that what floattype for non-Real types should return are not well defined.
For example, I am very confused what the floattype of TwicePrecision{T} should be. TwicePrecision{T} is not a Real, but Real-like. Well, Gray{T} is also Real-like.

Complex is also complex. Complex is a scalar type in some contexts, and also is a vector type in other contexts.

@kimikage
Copy link
Collaborator

floattype(::Type{<:AbstractIrrational}) = Float64 Or, BigFloat? 😵

@kimikage
Copy link
Collaborator

I'm contemplating releasing this as part of the 0.8 series, rather than as the beginning of a 0.9 series.

FixedPointNumbers is still in v0 series, so I think it is OK. However, v0.8 is starting to be used as the blocking by ColorTypes and Colors has been lifted. Therefore, it is better to make decisions early.

@timholy
Copy link
Member Author

timholy commented Mar 18, 2020

I thought about adding it for TwicePrecision but couldn't come up with a test that leveraged "real" functionality of this package (that is, doing something other than calling floattype directly). But perhaps I should break the horror and make a decision. Most recent commit contains my answers to these questions (debate welcomed).

timholy added 2 commits June 15, 2020 09:49
It seems to be a non-sequitur to allow `floattype` to return a type that
is not an `AbstractFloat`, unless it has been extended as such.
The docs have been enhanced to clarify when it is OK to extend `floattype`
in ways that don't return an `AbstractFloat`.

Breaking change: no, not counting the deprecation warning.
@timholy timholy force-pushed the teh/floattype_rational branch from e1c9cbf to 54fda32 Compare June 18, 2020 09:52
@timholy
Copy link
Member Author

timholy commented Jun 18, 2020

It's long overdue, but I'm finally ready to submit the implementation of JuliaGraphics/ColorVectorSpace.jl#126. I aim to merge dependencies in FixedPointNumbers and ColorTypes first, so expect a couple of PRs (like this one) that have languished to move into the "let's merge this" mode.

Copy link
Collaborator

@kimikage kimikage left a comment

Choose a reason for hiding this comment

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

These are trivial comments.
I think that changing Irrational to AbstractIrrational will cover all the subtypes of Real defined in Base.

src/FixedPointNumbers.jl Outdated Show resolved Hide resolved
src/FixedPointNumbers.jl Outdated Show resolved Hide resolved
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