Rearrangement of the structures by JulStraus · Pull Request #23 · sintefore/TimeStruct.jl

JulStraus · 2024-08-30T11:00:41Z

So far, it was difficult to keep an overview over the individual functions for all types. This is a first take on restructuring the folders and the files. I focused on representative periods as these are middle functions.

The aim of the restructuring is:

keep the current separation between the two main files for each type, that is the one providing the TimeStructure and its constructors as well as the corresponding TimePeriod (this would be, e.g., the file TwoLevel.jl) and the one which provides all internal types for iterating (this would be in the example the file strat_periods.jl),
provide separate files for the individual types created through calling, e.g., the function repr_period,
move all files for a given structure in a separate folder.

In addition, I ordered the method definitions for the individual functions so that they follow exactly the same structure and added docstrings.

Things to consider further:

Make OperationalPeriod <: TimePeriod a OperationalPeriod {P} <: TimePeriod where {P<:TimePeriod}

As a side note: I guess it is probably easier to directly look at the files and not the changes. The exception is most likely opscenarios.jl, stochastic.jl, and runtests.jl.

JulStraus · 2024-08-30T11:05:39Z

The problem in the x86 test is that I specified it to be Int64 in the comparison, while the documentation is not compiling due to internal references that are not available yet. This can be fixed when including an internal library.

trulsf · 2024-09-06T12:47:51Z

 include("simple.jl")
 include("calendar.jl")
-include("representative.jl")
+include(joinpath("representative", "core_types.jl"))


I do not think joinpath should be used here. It seems unnecessary in this setting, "representative/core_types.jl" is both shorter and easier to understand.

Fair enough. We switched to joinpath() a while ago. I guess the key reason from our side is to make it platform independent. Then again, we know that the tests are working with or without on both Linux and Windows.

trulsf · 2024-09-06T13:07:18Z

+periods of a `TimeStructure`. The type of the iterator is dependent on the type of the
+input `TimeStructure`,
+
+When the `TimeStructure` is a `TimeStructure`, `repr_periods` returns a


I would try to rephrase this a bit as the first sentence is kind of a tautology. I would also skip a reference to the SinglePeriodReprWrapper since this is documentation for an exported function. Maybe something along the lines of "If the time structure does not have representative periods, it will return the provided time structure as an iterable structure".

Regarding the first point: I push an update. I guess iterator for iterating is a bit too much.

I am a bit skeptical regarding the second point. I would anyhow suggest to export the internal docstrings as well to an internal library as we do in EMX.

trulsf · 2024-09-06T13:12:27Z

+"""
+When the `TimeStructure` is a [`RepresentativePeriods`](@ref), `repr_periods` returns the
+iterator [`ReprPeriods`](@ref).
+"""


As above, I am bit skeptical about referring to ReprPeriods which is an internal structure. Do we need a separate docstring for this version of repr_periods or should it be covered by one general docstring?

We could include it for the general. The advantage of individual is that it is easy to extend the docstring. Regarding the other points, see above.

trulsf · 2024-09-06T13:17:22Z

 _strat_per(sro::StratReprOpscenario) = sro.sp
 _rper(sro::StratReprOpscenario) = sro.rp

+mult_repr(sro::StratReprOpscenario) = sro.mult_rp


Should we export this function?

We could. It could be useful in general when we want to know the exact value within an representative period. In EMX, we however use the directly scaled value.

trulsf · 2024-09-10T14:17:43Z

I like the restructuring proposed as a template for TimeStruct as a whole. It would make it easier to navigate the code and easier to maintain. I guess this will add a bit more work also for the other parts (opscenarios, twolevel, twoleveltree).

I agree with the suggested change for OperationalPeriod to align with the other time period types.

Do you think this should be merged as a separate PR and the other time structures as individual PRs?

JulStraus · 2024-09-11T07:27:33Z

I like the restructuring proposed as a template for TimeStruct as a whole. It would make it easier to navigate the code and easier to maintain. I guess this will add a bit more work also for the other parts (opscenarios, twolevel, twoleveltree).

I agree with the suggested change for OperationalPeriod to align with the other time period types.

Do you think this should be merged as a separate PR and the other time structures as individual PRs?

I think we can manage it as a single major restructuring. I can get started with it rather soonish and can be finished within the end of next week. The question is whether we merge #22 first, as it would simplify a potential rebase.

Personally, I would also like to extend JuliaFormatter to 92 columns in this PR, as it anyhow leads to a lot of line changes. In this case, #22 definitely should be merged first.

JulStraus · 2024-09-17T12:21:01Z

I pushed a new update with also changing it to 92 column length in JuliaFormatter. Note that there are also changes with respect to types, hence definitely breaking.

I am not fully satisfied yet, but it is a move in the correct direction. There are still a few things which I want to test out.

JulStraus · 2024-09-18T06:15:33Z

I am not fully satisfied yet, but I think it is best to have separate PRs for the remaining parts. One important thing for me is to unify the type fields and potentially provide more functions there to access internal topics. I will create an Issue outlining the problems and think of an approach to the problems.

JulStraus added documentation Improvements or additions to documentation enhancement New feature or request labels Aug 30, 2024

JulStraus requested review from hellemo and trulsf August 30, 2024 11:00