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

Addition/subtraction for some AnchoredIntervals produces unexpected results #1

Closed
spurll opened this issue Feb 28, 2018 · 5 comments · Fixed by #6
Closed

Addition/subtraction for some AnchoredIntervals produces unexpected results #1

spurll opened this issue Feb 28, 2018 · 5 comments · Fixed by #6
Assignees

Comments

@spurll
Copy link
Contributor

spurll commented Feb 28, 2018

Arithmetic involving an AnchoredInterval{P, T} when P isa Period runs afoul of the rounding code in the constructor, producing counterintuitive results:

julia> he = HourEnding(now())
HourEnding{DateTime}(2018-02-28T18:00:00, Inclusivity(false, true))

julia> he - Minute(30)
HourEnding{DateTime}(2018-02-28T18:00:00, Inclusivity(false, true))

julia> he + Minute(30)
HourEnding{DateTime}(2018-02-28T19:00:00, Inclusivity(false, true))

Rounding will have to be removed from the AnchoredInterval{P, T} constructor to resolve this issue, but it is nice being able to call HourEnding(now()) without having to explicitly round now() to the nearest hour.

Of the many possible solutions @omus and I have considered, two seem most promising:

  1. HourEnding{T} remains a type alias for AnchoredInterval{Hour(-1), T}, but only the HourEnding constructor will perform the rounding operation
    • This means that HourEnding{DateTime}(now()) will not be rounded (as it's just an alias for AnchoredInterval{Hour(-1), DateTime}), which may lead to confusion
  2. HourEnding and AnchoredInterval both become subtypes of a new AbstractAnchoredInterval (Curt has suggested SpanInterval as a replacement name)
    • This doesn't sit particularly well with me because, aside from the rounding that happens in the constructor, the functionality will be identical

Either way, I'll have to fix the way string(::AnchoredInterval{P, T}) works, because currently it assumes that everything will be rounded when T <: TimeType.

@spurll spurll self-assigned this Feb 28, 2018
@spurll
Copy link
Contributor Author

spurll commented Feb 28, 2018

My plan is to proceed with solution 1 for now, and consider 2 in the future.

@omus
Copy link
Collaborator

omus commented Mar 2, 2018

This means that HourEnding{DateTime}(now()) will not be rounded (as it's just an alias for AnchoredInterval{Hour(-1), DateTime}), which may lead to confusion

One way to support HourEnding{DateTime}(now()) to be rounded while having AnchoredInterval{Hour(-1), DateTime}) not be rounded while still using type aliases would be to introduce an additional type parameter.

MWE:

using Base.Dates
using Base.Dates: TimeType
const AbstractDateTime = TimeType

struct AnchoredInterval{P, T, R}
    anchor::T
end

AnchoredInterval{P, T}(i::T) where {P, T} = AnchoredInterval{P, T, false}(i)

const HourEnding{T} = AnchoredInterval{Hour(-1), T, true} where T <: AbstractDateTime
HourEnding{T}(i::T) where T <: AbstractDateTime = invoke(HourEnding{T}, Tuple{Any}, ceil(i, Hour))

HourEnding{DateTime}(now())
AnchoredInterval{Hour(-1), DateTime}(now())

@omus
Copy link
Collaborator

omus commented Mar 2, 2018

I think I would prefer option 2 however.

@spurll
Copy link
Contributor Author

spurll commented Mar 2, 2018

It seems to me that the third type parameter is likely to cause a lot of confusion.

How do you feel about something like:

AbstractInterval{T} (abstract)
↳ Interval{T} (concrete)
↳ AbstractAnchoredInterval{P, T} (abstract)
  ↳ AnchoredInterval{P, T} (concrete)
  ↳ RoundedAnchoredInterval{P, T} (concrete)
    ↳ HourEnding{T} (type alias for RoundedAnchoredInterval{Hour(-1), T})
    ↳ HourBeginning{T} (type alias for RoundedAnchoredInterval{Hour(1), T})

Names subject to change, of course.

@omus
Copy link
Collaborator

omus commented Mar 2, 2018

I like that proposal. My only issue is that I think AbstractAnchoredInterval will be used allot and it a little long.

spurll added a commit that referenced this issue Mar 6, 2018
Remove rounding from `AnchoredInterval{P}` (which was a special case when
`P` is a `Period`) and replace it with the pseudoconstructors `HE` and
`HB`. This fixes some problems associated arithmetic involving
`AnchoredInterval`s.

Also `intersect(AnchoredInterval, AnchoredInterval)` now returns an
`AnchoredInterval` instead of an `Interval` (but the tests are broken;
they'll be fixed once we change the value type parameter to a field).

Closes #1
@omus omus closed this as completed in #6 Mar 7, 2018
omus pushed a commit that referenced this issue Mar 7, 2018
* Remove rounding from AnchoredInterval

Remove rounding from `AnchoredInterval{P}` (which was a special case when
`P` is a `Period`) and replace it with the pseudoconstructors `HE` and
`HB`. This fixes some problems associated arithmetic involving
`AnchoredInterval`s.

Also `intersect(AnchoredInterval, AnchoredInterval)` now returns an
`AnchoredInterval` instead of an `Interval` (but the tests are broken;
they'll be fixed once we change the value type parameter to a field).

Closes #1

* Document rounding changes to HourEnding

* Add tests for `in(::Interval, ::Interval)`

* Updates as per code review

* Fix broken tests, improve coverage

* Rename promote_period to canonicalize
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 a pull request may close this issue.

2 participants