Add support for Dates.FixedPeriod and Dates.CompoundPeriod

[sostock]

This is an attempt at #125.

Things to (maybe) add:

• Documentation
• Support for CompoundPeriods (if they only contain FixedPeriods)

[codecov-io]

Codecov Report

Merging #331 into master will increase coverage by 0.45%.
The diff coverage is 93.93%.

@@            Coverage Diff             @@
##           master     #331      +/-   ##
==========================================
+ Coverage   80.23%   80.69%   +0.45%     
==========================================
  Files          15       16       +1     
  Lines        1174     1207      +33     
==========================================
+ Hits          942      974      +32     
- Misses        232      233       +1     

Impacted Files	Coverage Δ
src/Unitful.jl	100.00% <ø> (ø)
src/dates.jl	93.93% <93.93%> (ø)
src/conversion.jl	85.48% <0.00%> (+1.61%)	⬆️

---

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 e97233b...024a6ff. Read the comment docs.

[codecov-commenter]

Codecov Report

Merging #331 into master will increase coverage by 1.02%.
The diff coverage is 95.58%.

@@            Coverage Diff             @@
##           master     #331      +/-   ##
==========================================
+ Coverage   79.74%   80.76%   +1.02%     
==========================================
  Files          15       16       +1     
  Lines        1180     1248      +68     
==========================================
+ Hits          941     1008      +67     
- Misses        239      240       +1     

Impacted Files	Coverage Δ
src/Unitful.jl	100.00% <ø> (ø)
src/dates.jl	95.58% <95.58%> (ø)
src/quantities.jl	93.56% <0.00%> (ø)
src/units.jl	83.56% <0.00%> (+0.68%)	⬆️
src/conversion.jl	85.48% <0.00%> (+1.61%)	⬆️

---

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 ea34378...2bd6016. Read the comment docs.

[sostock]

I now added support for CompoundPeriods. They can be converted to Quantitys using convert/uconvert, but don’t support all operations that are supported by FixedPeriods. For example:

• unit(::CompoundPeriod) errors, while unit(::FixedPeriod) returns the appropriate unit (e.g., unit(Nanosecond(5)) == u"ns").
• Quantity(::CompoundPeriod) errors, while Quantity(::FixedPeriod) returns the appropriate quantity (e.g., Quantity(Nanosecond(5)) == 5u"ns").
• Arithmetic operations with CompoundPeriods error if the resulting unit would depend on the input units, e.g.:
  • +(::Quantity, ::CompoundPeriod) and +(::CompoundPeriod, ::Quantity) error.
  • div(::Quantity, ::CompoundPeriod) and div(::CompoundPeriod, ::Quantity) work, since the result is dimensionless.
  • mod(::CompoundPeriod, ::Quantity) works while mod(::Quantity, ::CompoundPeriod) does not, since the second argument determines the output unit.

We could either leave it like that, or decide between two options to enable all arithmetic operations between Quantitys and CompoundPeriods:

• Define unit(::CompoundPeriod) = u"s" and treat it as such in all arithmetic operations. This is consistent with the addition between quantities with different time units: 5u"ms" + 1u"hr" == (720001//200)u"s"
• Don’t define unit(::CompoundPeriod), but convert the CompoundPeriod to the unit of the Quantity when used in an arithmetic operation between a CompoundPeriod and Quantity.

[sostock]

I now added documentation. If we are ok with the behavior described in #331 (comment), this should be good to review and merge.

[OliverEvans96]

What's the current status of this PR? Is there more work that needs to be done before it can be merged? If some of the above issues are deal-breakers, I could take a shot at addressing them.

[sostock]

If we are fine with the current behavior (see #331 (comment)), then this should be good to merge.

One issue remains: since Periods and Quantitys can now compare equal, they should also hash the same. But hashing is broken for this package anyway, so I didn’t address that for now. Hashing can only be repaired by changing the promotion behavior of logarithmic quantities, which I would consider breaking (see #402). For previous discussion on the hashing issue see #378, #379.

[codecov-io]

Codecov Report

Merging #331 (2bd6016) into master (ea34378) will increase coverage by 4.81%.
The diff coverage is 95.77%.

@@            Coverage Diff             @@
##           master     #331      +/-   ##
==========================================
+ Coverage   79.74%   84.55%   +4.81%     
==========================================
  Files          15       16       +1     
  Lines        1180     1554     +374     
==========================================
+ Hits          941     1314     +373     
- Misses        239      240       +1     

Impacted Files	Coverage Δ
src/Unitful.jl	100.00% <ø> (ø)
src/dates.jl	95.77% <95.77%> (ø)
src/display.jl	95.53% <0.00%> (-0.39%)	⬇️
src/user.jl	95.65% <0.00%> (+0.61%)	⬆️
src/fastmath.jl	84.50% <0.00%> (+1.96%)	⬆️
src/quantities.jl	95.87% <0.00%> (+2.30%)	⬆️
src/utils.jl	95.34% <0.00%> (+2.32%)	⬆️
src/promotion.jl	43.47% <0.00%> (+3.47%)	⬆️
... and 7 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 ea34378...2bd6016. Read the comment docs.

[OliverEvans96]

It seems like fixing arithmetic would be good. I would definitely be surprised to find that it doesn't work.

I'm having a hard time deciding which of your solutions I like better. On one hand, it seems a little strange to say unit(::CompundPeriod) = seconds since it doesn't really just have a single unit. And would this option lead to any InexactErrors for CompoundPeriods which are not integer multiples of a second?

But on the other hand, converting to the units of the Quantity feels a little bit too magical, specifically in the case of mod. That definitely seems like an edge-case, but good to get it right.

Does anyone else have thoughts?

[cstjean]

It seems like fixing arithmetic would be good. I would definitely be surprised to find that it doesn't work.

My 2 cents is that this is already a big PR, which is overdue for a review and merge. The proposed arithmetic implementation doesn't look "obviously right" to me, so it seems wiser to leave it to a future PR than to risk having to deal with a breaking change if a mistake is made.

[giordano]

Looks good to me. I think it's better to not add more complexity here. If there are no objections, I'll merge tomorrow

[ericphanson]

mind tagging a release with this feature?

[giordano]

Done, this is in v1.7.0

[ericphanson]

thanks!