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 functions to compute time resolution periods and matrices #144

Merged
merged 3 commits into from
Oct 23, 2023

Conversation

abelsiqueira
Copy link
Member

@abelsiqueira abelsiqueira commented Oct 17, 2023

Pull request details

Describe the changes made in this pull request

One assumption is made: the description of a time period is a range, i.e., an object start:end.
Creates a function compute_rp_periods that receives a list of time steps (such as the time steps of various flows), and computes the periods for that representative period.
Creates a function resolution_matrix that computes the matrix of a given flow/asset in a given representative period.

List of related issues or pull requests

Closes #109

Collaboration confirmation

As a contributor I confirm

  • I read and followed the instructions in README.dev.md
  • The documentation is up to date with the changes introduced in this Pull Request (or NA)
  • Tests are passing
  • Lint is passing

@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

All modified lines are covered by tests ✅

Files Coverage Δ
src/TulipaEnergyModel.jl 100.00% <ø> (ø)
src/time-resolution.jl 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@abelsiqueira
Copy link
Member Author

The coverage fails are from input_tables.jl, which I have not touched. We haven't discussed a coverage acceptance criteria, but either way, it is not because of this PR.

Copy link
Member

@clizbe clizbe left a comment

Choose a reason for hiding this comment

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

Thanks looks great! I think Diego will have to check the second function.

src/time-resolution.jl Show resolved Hide resolved
time_step in time_steps
]

return M
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to call it M (which I assume matches the paper) or something more descriptive?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm gonna change to matrix, although it is not going to be visible anywhere outside this function.

src/time-resolution.jl Show resolved Hide resolved
@abelsiqueira abelsiqueira marked this pull request as draft October 18, 2023 12:35
@abelsiqueira abelsiqueira marked this pull request as ready for review October 18, 2023 13:50
Notice that this implies that they form a disjunct partition of `1:N`.

The output will also be an array of ranges with the conditions above.
The output is constructed greedly, i.e., it selects the next largest breakpoint following the algorithm below:
Copy link
Member

Choose a reason for hiding this comment

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

"greedily"

Comment on lines +80 to +90
time_steps1 = [1:4, 5:8, 9:12]
time_steps2 = [1:3, 4:6, 7:9, 10:12]
compute_rp_periods([time_steps1, time_steps2])

# output

3-element Vector{UnitRange{Int64}}:
1:4
5:8
9:12
```
Copy link
Member

Choose a reason for hiding this comment

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

Is there a more complex example that would explain more?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a more complex example, but I don't think it explains more. Let me know what would help.

Copy link
Member

@clizbe clizbe left a comment

Choose a reason for hiding this comment

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

LGTM! Might still want Diego to check it over.

)
period_end = maximum(breakpoints)
@assert period_end ≥ period_start
push!(rp_periods, period_start:period_end)
Copy link
Member

Choose a reason for hiding this comment

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

I just read what 'push!' does, but won't this link the values with period_start & period_end so they'll update as you change them?
So instead of [1:3, 4:6] you'd get [4:6, 4:6]?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. That might happen if the period_start variable was an array-like (or pointer)-like object. In which case you would need to make a copy. But in this case that is not necessary.

For instance

L = []
v = ones(3)
push!(L, v) # Now L = [[1.0, 1.0, 1.0]]
v[1] = -1 # Now L = [[-1.0, 1.0, 1.0]]
push!(L, v) # Now L = [[-1.0, 1.0, 1.0], [-1.0, 1.0, 1.0]]

That happens because v is an array and pushing it into L pushes the "pointer" to it. Doing push!(L, copy(v)) would fix it.

However, this would work:

L = []
v = [1, 1, 1]
push!(L, v)
v = [-1, 1, 1]
push!(L, v)

Because the second v is not the same "pointer" as the first. L has two elements, one is the pointer to the first v, and the second is the pointer to the second v.

In other words, pointers are complicated.

@abelsiqueira abelsiqueira requested a review from clizbe October 19, 2023 13:57
@abelsiqueira
Copy link
Member Author

Hi Lauren, I've made some modifications. Let me know if there is something that would help more.
Also, feel free to use "Request changes" instead of "Approve" if you want another pass on the PR.

Copy link
Member

@clizbe clizbe left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for adding the complex example - I think it adds some explanation. :)

@clizbe clizbe merged commit 71f8d80 into TulipaEnergy:main Oct 23, 2023
5 checks passed
@abelsiqueira abelsiqueira deleted the 109-matrix branch October 24, 2023 09:46
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.

Implement a function that generates the resolution matrix
2 participants