-
Notifications
You must be signed in to change notification settings - Fork 33
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
Missing iteration over number of profiles in algorithm description #32
Comments
I noticed it while refactoring the code to eliminate copy pasting and making it more readable to newcomer in rl-institut#1 |
Hi, thanks for bringing this up. Yes, step 1 is only done once and is not iterated over each (daily) profile. I think this is a bit implicit in the paper and comes across more clearly only in the paragraph following the procedure description. In such a paragraph, we discuss more specifically the way to calculate the peak time frame, from which it is clear that no parameter would change across subsequent daily profiles (and so that there's no point repeating it). More generally, the description of the procedure in the paper that you have copy-pasted here was not meant to be a super-precise algorithmic description of the workflow, but rather a qualitative description to give a general understanding. The code (which was/is open and linked to the publication) remains the reference for any details about the algorithm. Actually, the code has changed quite a lot since the original paper, so we should definitely add such details in the README here if we deem them important. Code changes, papers don't! Would you like to add an "Algorithm" section to the documentation with a PR? That would be very helpful. |
Yes I could do this :) |
Here is the algorithm reproduced here form https://doi.org/10.1016/j.energy.2019.04.097 for convenience
Should we then repeat 1. -3. for each profile of
range(num_profile)
?If yes then step 1 is only done once once before iterating over range(num_profile)
Otherwise step 2. should mention a loop over the number of profiles as well
The text was updated successfully, but these errors were encountered: