-
Notifications
You must be signed in to change notification settings - Fork 78
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
Issue494 baselinetesting #495
Issue494 baselinetesting #495
Conversation
Merge for v0.6.0 release
Update copyright year
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.
@terrancelu92 This is looking very good and much better, thanks again for the updates. I have some additional comments in-line below. In addition to those, please also address the following:
- Update your branch to include the latest ibpsa:master.
- Change
baseline_testing
directory name tobaselines
since its more concise. - Add a brief description of the change/addition in
releasenotes.md
under the 0.6.0-dev heading. You can look at the previous entries for an idea of what to write. - Add your name to
contributors.md
.
baseline_testing/run_baselines.py
Outdated
model_config = config[test_case] | ||
|
||
# Run user-defined baseline control testing or run all scenarios | ||
if model_config.get("run_user_defined_test", False): |
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.
Are you sure this is valid since model_config, defined in Line 137, I think only contains fields in the specific test case, which are electricity_price and time_period? Is "run_user_defined_test" checked correctly?
Related, it would be good to include the script(s) and config(s) you used to generate all the two-week rolling simulations (if they're pretty concise), which I think probably used the "user_defined" feature and maybe automated creation of the necessary config files? Were they tested with this version of run_baselines.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.
During this round of revision, I've tested both the "user_defined" feature with "true" and "false" (running predefined scenarios) settings. Additionally, I've included a script baselines/csv/run_all_scenarios.py
for running two-week rolling simulations to reproduce the baseline testing results.
Thank you @dhblum for the review! I will address these within a few days. |
@dhblum , thank you for all the comments. I have addressed the following comments and also the in-line ones. Please take another review and let me know further edits if needed. Thank you!
|
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.
@terrancelu92 Thank you again for addressing my comments. Can you please address just a few more I've made, in-line? We're getting close and I appreciate your effort to complete this.
@dhblum Thank you for providing detailed instructions and comments. I have incorporated the changes as per your in-line comments. Would you mind reviewing the updated version once more? Please let me know if there are any further adjustments needed. Thank you once again for your guidance! |
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 @terrancelu92!
The testcase driver scripts for running baseline controllers and associated results files are included.