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 intersect method #496

Closed
wants to merge 6 commits into from

Conversation

guilhermebodin
Copy link

No description provided.

src/timeaxis/timegrid.jl Outdated Show resolved Hide resolved
src/timeaxis/timegrid.jl Outdated Show resolved Hide resolved
src/timeaxis/timegrid.jl Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jun 13, 2021

Codecov Report

❗ No coverage uploaded for pull request base (v0.30@15c3bd7). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##             v0.30     #496   +/-   ##
========================================
  Coverage         ?   85.87%           
========================================
  Files            ?       17           
  Lines            ?      899           
  Branches         ?        0           
========================================
  Hits             ?      772           
  Misses           ?      127           
  Partials         ?        0           

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 15c3bd7...75f0468. Read the comment docs.

src/timeaxis/timegrid.jl Outdated Show resolved Hide resolved
src/timeaxis/timegrid.jl Outdated Show resolved Hide resolved
src/timeaxis/timegrid.jl Outdated Show resolved Hide resolved
new_o = max(tg_inf1.o, tg_inf2.o)
return TimeGrid(new_o, tg_inf1.p)
end
function intersect(tg_fin1::TimeGrid{T,P,:finite}, tg_fin2::TimeGrid{T,P,:finite}) where {T,P}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I found a case that might yield empty TimeGrid:

julia> tg = TimeGrid(DateTime(2021, 1, 1, 0), Hour(1), 24)
24-element TimeGrid{DateTime, Hour, :finite}:
 2021-01-01T00:00:00
  
 2021-01-01T23:00:00
 / 1 hour

julia> tg′ = TimeGrid(DateTime(2021, 1, 1, 0, 30), Hour(1), 24)
24-element TimeGrid{DateTime, Hour, :finite}:
 2021-01-01T00:30:00
  
 2021-01-01T23:30:00
 / 1 hour

julia> intersect(collect(tg), collect(tg′))
DateTime[]

Copy link
Author

Choose a reason for hiding this comment

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

In fact I was wrong on slack there is a way to infinite with infinite intersection being empty

Copy link
Author

Choose a reason for hiding this comment

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

I rewrote some parts and added more tests.

The current behavior of intersecting tg with different periods is method error. What do you think about that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The current behavior of intersecting tg with different periods is method error. What do you think about that?

That might need IrregularTimeGrid implemented.
I think we can revisit it later, putting a TODO here is fine for me.

I still don't make a perfect algo come out. Here is my two cents:

  • For the case tg.p <: Period (or maybe narrow down to FixedPeriod ?), treat all the period as Int and we can get a TimeGrid.
  • For the case tg.p <: Float64 (and Irrational included. Although I know π is marked a Irrational type, all calculation on it are approximations.), just fallback to the intersect(::Vector, ::Vector) calculation and output IrregularTimeGrid.

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