-
Notifications
You must be signed in to change notification settings - Fork 1
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
Bump dependency versions #15
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally speaking ok. Seems like not too many changes. I would however highly suggest to limit the compatibility to the required values to avoid any confusions.
@@ -219,14 +219,14 @@ function get_periods(T::TS.TimeStructure, type::Type, sp::Int64, rp::Int64, sc:: | |||
if eltype(T.operational[sp].rep_periods) <: TS.OperationalScenarios | |||
return [ | |||
t for | |||
t ∈ T if t.sp == sp && t.period.rp == rp && t.period.period.sc == sc | |||
t ∈ T if t.sp == sp && t.period.rp == rp && t.period.period.osc == sc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to try to find a different design here. Although I am quite certain we are in TimeStruct
now at a state that we do no longer change it, it is still problematic to go directly for the individual fields. This is relevant for all methods for the function get_periods()
.
On a side note, do we still need get_periods(T::TS.TimeStructure, type::Type)
. It is at least not called within results_axis_utils.jl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I considered using internal functions in TimeStruct for this, but I guess that is also problematic?
get_periods(T::TS.TimeStructure, type::Type)
is called in src/utils_GUI/GUI_utils.jl
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it is equally problematic. There are other approaches how we can handle that using the Parametric input type. We can revisit it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the tests have run, you can merge it.
Bumped versions to be compatible with TimeStruct v0.9 and EnergyModelsBase v0.8.1. Also adjusted the topology legend and title to always be visible. Removed redundant compatibility requirements for tests.