-
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
Extending final_time by 1 hour for EPW radiation data #151
base: master
Are you sure you want to change the base?
Conversation
…lation gives consistent result for final value in timeseries
I think this is a good fix in general, except for the case where the user wants data for the end of the year. In this case, the data stops at 12/31 22:00:00. The df_epw goes from 1/1 00:00:00 to 12/31 23:00:00 to be consistent with the buildings library. The +pd.Timedelta('1 hours') does not get any more data for the last hour, and then the data is shifted back by 30 min with no additional value added to the end (I think this was part of the reason for my original implementation since solar data is always 0 around midnight - though you've correctly pointed out the flaw for all other times during the year). Perhaps a fix is if the final time is after 12/31 22:00:00, repeat the value at the end? |
Thanks for looking at the PR and that's a good point about the end of .epw datasets. Repeating the value on the end would be fine I think, however this would disrupt the daily cycle (very slightly) which may be of greater interest for MPC. Most of the data being used is TMY data anyway so cycling the dataset back to 01/01 00:00:00 (of the same dataset) is perhaps more expected. If the dataset continues to the next year then reading this would be no issue, as in a real data case. A check could be added at date>=12/31 22:00:00 reading +pd.Timedelta('1 hours') yields a result, if false restart from the beginning of the datafile. A more elegant solution could be during the weather data import checking that the dataset continues at least to the end of the MPC experiment, then adding an option to append the TMY data with the year incremented if .epw dates do not span the MPC duration. This would view the issue more as making sure the predictive model data generation is correct. Let me know what you think and I can revise the PR. |
It would only disrupt the daily cycle for MPC if data is needed for days after the epw file ends. This issue does not happen with real data from CSV or DF. This actually is an issue anyway to deal with what happens if a user wants to run a simulation of MPC control to the very end of the year - what happens with weather forecasts from epw files that are required for control on 12/31? Or, I guess, what should happen? I can't find whether it is reasonable to assume the data from 12/31/YR and 1/1/YR+1 could be assumed continuous. The Buildings Library actually states that it may not be, hence the repetition of the first value for midnight of 1/1/YR. @mwetter Do you have thoughts on this? |
@TStesco I think the implementation should append data from the beginning of the year to the end of the year if the final_time crosses into the new year relative to the start_time. |
Sounds good, I'll take a stab at implementing this next week. |
Hey @TStesco, just wondering if you've had a chance to look at this again? |
Hi @dhblum, I've been pretty swamped with my thesis work unfortunately. If you want to fix the issue don't wait for me, but I can likely get to it early-mid October after my submission/defence otherwise. |
Ok, thanks. No problem, if I went ahead with it I wanted to make sure I wasn't duplicating work. Good luck with your thesis! |
Extending final_time by 1 hour for EPW radiation data so that interpo…lation gives consistent result for final value in timeseries. This means that appending the final uninterpolated value is no longer required.
See Issue.
For the tests to pass the data files in the unttests.WeatherFromEPW would need to be updated also: collect_data.csv, collect_data_partial_display.csv, collect_data_partial_base.csv
If you think this should be handled in a different way let me know.