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 InfExtendedTime for TimeType support #10

Merged
merged 9 commits into from
Jun 10, 2020

Conversation

fchorney
Copy link
Contributor

@fchorney fchorney commented Jun 4, 2020

Alright here's a first pass at attempting to add TimeType support.
Tried to take #4 and #5 into account.

Closes #3

@@ -0,0 +1,16 @@
Base.isfinite(x::InfExtendedTime) = x.flag == finite && isfinite(x.finitevalue)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know that I really need that isfinite(x.finitevalue) to be there.

Copy link
Contributor

@omus omus left a comment

Choose a reason for hiding this comment

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

I suspect most of this code was copied from InfExtended. It would probably be best to do some code generation rather than copy/paste. For example:

for InfExt in (InfExtended, InfExtendedTime)
    @eval begin 
        Base.promote_rule(::Type{$InfExt{T}}, ::Type{$InfExt{S}}) where {T, S} = $InfExt(promote_type(T, S))
        Base.promote_rule(::Type{$InfExt{T}}, ::Type{S}) where {T, S} = $InfExt(promote_type(T, S))
        Base.promote_rule(::Type{$InfExt{T}}, ::Type{Infinite}) where T = $InfExt{T}
    end
end

src/Infinity.jl Outdated Show resolved Hide resolved
src/infextendedtime/base.jl Outdated Show resolved Hide resolved
src/infextendedtime/base.jl Outdated Show resolved Hide resolved
src/infextendedtime/base.jl Outdated Show resolved Hide resolved
Base.isfinite(x::InfExtendedTime) = x.flag == finite && isfinite(x.finitevalue)
Base.isinf(x::InfExtendedTime) = isposinf(x) || isneginf(x)
Base.:(==)(x::InfExtendedTime, y::InfExtendedTime) = (isfinite(x) && isfinite(y)) ? x.finitevalue == y.finitevalue : x.flag == y.flag
Base.hash(x::InfExtendedTime, h::UInt) = isfinite(x) ? hash(x.finitevalue, h) : hash(x.flag, h)
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently wouldn't support hashing different infinities (e.g. InfExtended vs. InfExtendedTime) to the same result. I'm not sure what the desired behaviour is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

Copy link
Owner

Choose a reason for hiding this comment

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

I'd like the hash to simply defer to whatever the true value is, i.e. the finitevalue or PosInf or NegInf. It's fine that the infinite date has the same hash as the infinite number.

src/infextendedtime/comparison.jl Outdated Show resolved Hide resolved
@fchorney
Copy link
Contributor Author

fchorney commented Jun 4, 2020

I suspect most of this code was copied from InfExtended. It would probably be best to do some code generation rather than copy/paste. For example:

for InfExt in (InfExtended, InfExtendedTime)
    @eval begin 
        Base.promote_rule(::Type{$InfExt{T}}, ::Type{$InfExt{S}}) where {T, S} = $InfExt(promote_type(T, S))
        Base.promote_rule(::Type{$InfExt{T}}, ::Type{S}) where {T, S} = $InfExt(promote_type(T, S))
        Base.promote_rule(::Type{$InfExt{T}}, ::Type{Infinite}) where T = $InfExt{T}
    end
end

I guess this kind of breaks down the separated structure that #5 was looking for. Though I guess a few things could be moved into a more generic section. I'm thinking maybe this could be part of #5?

@omus
Copy link
Contributor

omus commented Jun 4, 2020

I'm thinking maybe this could be part of #5?

Very true, maybe then it would good to add comments as to parts that were copied from InfExtended but had the type changed to InfExtendedTime? Would make it easier to catch mistakes

@cjdoris
Copy link
Owner

cjdoris commented Jun 6, 2020

Thanks for this, it looks really good. Please merge in the master branch --- I renamed the (badly thought through) InfFlag constants to be uppercase (e.g. POSINF), because they conflicted with things from Infinity.Utils, which will break your code somewhere.

@cjdoris
Copy link
Owner

cjdoris commented Jun 6, 2020

I'm thinking maybe this could be part of #5?

Very true, maybe then it would good to add comments as to parts that were copied from InfExtended but had the type changed to InfExtendedTime? Would make it easier to catch mistakes

If there is code that is sensible to generate together for both InfExtended (which I'm considering renaming to InfExtendedReal) and InfExtendedTime then please do. Maybe there should be directories infextendedcommon, infextendedreal and infextendedtime.

@codecov-commenter
Copy link

codecov-commenter commented Jun 8, 2020

Codecov Report

Merging #10 into master will increase coverage by 13.16%.
The diff coverage is 90.62%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #10       +/-   ##
===========================================
+ Coverage   54.23%   67.40%   +13.16%     
===========================================
  Files          13       18        +5     
  Lines         118      181       +63     
===========================================
+ Hits           64      122       +58     
- Misses         54       59        +5     
Impacted Files Coverage Δ
src/Infinity.jl 100.00% <ø> (ø)
src/utils.jl 100.00% <ø> (ø)
src/infextendedtime/conversion.jl 57.14% <57.14%> (ø)
src/infextendedtime/arithmetic.jl 91.30% <91.30%> (ø)
src/infextendedtime/comparison.jl 93.75% <93.75%> (ø)
src/infextendedtime/base.jl 100.00% <100.00%> (ø)
src/infextendedtime/io.jl 100.00% <100.00%> (ø)
src/infinite/base.jl 75.00% <100.00%> (+8.33%) ⬆️
src/infinite/conversion.jl 53.84% <0.00%> (-3.30%) ⬇️
... and 5 more

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 b50a1a5...29b02e0. Read the comment docs.

@cjdoris
Copy link
Owner

cjdoris commented Jun 10, 2020

Thanks :)

@cjdoris cjdoris reopened this Jun 10, 2020
@cjdoris cjdoris merged commit f0f7037 into cjdoris:master Jun 10, 2020
@cjdoris
Copy link
Owner

cjdoris commented Jun 10, 2020

It helps if I click the merge button!

@fchorney fchorney deleted the fc/infextendedtime branch June 15, 2020 15:01
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.

Support Date, DateTime
4 participants