-
Notifications
You must be signed in to change notification settings - Fork 70
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
Issue135 forecast uncertainty #482
Issue135 forecast uncertainty #482
Conversation
ErrorEmulator.py: function to generate an error with an AR model in the prediction horizon. SimulateError.ipynb: notebook to simulate the error for different uncertainty scenarios.
Thanks a lot for the PR, @laura-zabala! I've gone through your additions and they are looking great. As I see it, now we have two main options to move forward:
My opinion is that we should go for 1. In that case, the steps I suggest to follow (may be missing some) are:
|
TY for your feedback Javier! I also think it's better to go for option 1. I'll check your suggestions |
The predict_error method is imported in forecaster.py.
The arguments required by "predict_error" are propagated in get_forecast: F0, K0, F, K, mu. The hp is already defined (horizon). Default values are included for these parameters for the deterministic scenario.
Additional step:
|
Hey all, thanks for the PR and comments already! Just my thoughts here, sorry they're a bit late. But I also vote for 1. However, I might suggest being a little more strategic in preparation for other variables, and in particular other error models. I suggest calling I'm also thinking the approach of propagating parameters directly to |
predict_error is updated to predict_error_AR to specify the error model
I updated the |
@laura-zabala Thanks. Sure, let's keep working this way through all what's needed from top to bottom to implement and keep track of some of these "to-dos." |
Additional step:
|
…into issue135_forecastUncertainty # Conflicts: # forecast/forecaster.py
…nario, and prevent the weather forecast uncertainty from being overwritten in get_forecast after setting it in the scenario.
…n and introduced two new tests: test_forecast_temperature_are_within_range and test_forecast_solar_radiation_are_positive for improved criteria checking.
Issue135 forecast uncertainty
@wfzheng I reviewed the implementation of the function
To-do's from my side:
|
@wfzheng In the |
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.
Thank you @laura-zabala and @wfzheng for this implementation! I've started reviewing and left some inline comments. Can you start to address? I might have additional comments soon after another review and some thinking more on the structure of the implementation, but these are a start.
!
Outdated
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.
Should not be committed.
@@ -15,3 +15,7 @@ testcase2/doc/build/ | |||
parser/*.fmu | |||
parser/*.mo | |||
parser/*.json | |||
|
|||
xx.ipynb | |||
connect_boptest.py |
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.
I don't see a connect_boptest.py and don't think this should be written in the .gitignore.
|
||
xx.ipynb | ||
connect_boptest.py | ||
\! |
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.
This shouldn't be in the .gitignore.
@@ -15,3 +15,7 @@ testcase2/doc/build/ | |||
parser/*.fmu | |||
parser/*.mo | |||
parser/*.json | |||
|
|||
xx.ipynb |
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.
I don't see why this should be in the .gitignore.
@@ -30,45 +33,72 @@ def __init__(self, testcase): | |||
# Point to the test case object | |||
self.case = testcase | |||
|
|||
def get_forecast(self,point_names, horizon=24*3600, interval=3600, | |||
def get_forecast(self, point_names, horizon=24 * 3600, interval=3600, | |||
weather_temperature_dry_bulb=None, weather_solar_global_horizontal=None, seed=None, |
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.
I suggest shorter variable names, using three letters, such as: wea_tem_dry_bul
instead of the full words as you have.
Also missing documentation of these parameters, which should be included as docstrings as in other functions, and was done previously for this one.
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.
Thank you for your suggestions regarding the variable names, the documentation and the issue of '.gitignore' file. I have updated the variable names to shorter versions as you suggested, updated the docstrings and corrected the '.gitignore'.
I've created a pull request to merge these modifications into the issue135_forecastUncertainty branch of the forked repository at laura-zabala/project1-boptest.
indices = np.where(original_HGloHor > 50)[0] | ||
|
||
|
||
for i in range(200): |
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.
Is it ever the case 200 is not enough? What happens if condition not met after 200?
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.
- The limit of 200 iterations is set to balance between accuracy and computational efficiency. Increasing the number of iterations could slow down the simulation.
- If the condition is not met after 200 iterations, the code still ensures that the forecast values remain within the specified range by applying the np.clip function. [Line 99]
@laura-zabala > * |
…o issue135_forecastUncertainty merge laura's revise
Hey @laura-zabala, can you:
Then I think I'll be able to review things again more easily within this PR. |
Issue135 forecast uncertainty
Issue135 forecast uncertainty
Hi, |
@laura-zabala I'm closing this PR in favor of #693 since for some reason this one seems to leave out recent changes by @wfzheng. |
The function for the uncertainty emulator and an example notebook for application are included.