NREL / openstudio-calibration-gem

Other
5 stars 5 forks source link

TimeseriesObjectiveFunction fails because of missing year argument #27

Open MatthewSteen opened 3 years ago

MatthewSteen commented 3 years ago

The year from the CSV file needs to be added as an argument to the MonthOfYear call in line 546 (or use line 544 instead).

https://github.com/NREL/openstudio-calibration-gem/blob/f45c3cafccc06035403b31ece0b64d794a74f75f/lib/measures/TimeseriesObjectiveFunction/measure.rb#L535-L547

Without it, OpenStudio assumes 2009, which fails the >= conditional below for timestamps after 2009.

https://github.com/NREL/openstudio-calibration-gem/blob/f45c3cafccc06035403b31ece0b64d794a74f75f/lib/measures/TimeseriesObjectiveFunction/measure.rb#L554-L559

Experienced this issue with timeseries data in the following format.

datetime,occ_zone_temp_ave,plm_zone_temp_ave, 10/30/2020 0:00,25.61841766,23.6810688, 10/30/2020 0:01,25.61639483,23.692894, 10/30/2020 0:02,25.62348463,23.70474212, 10/30/2020 0:03,25.62786781,23.69617423, 10/30/2020 0:04,25.62711445,23.69417523, 10/30/2020 0:05,25.63245887,23.69228997,

MatthewSteen commented 3 years ago

Another useful update for this measure would be to remove " from the path argument, which should be added to all measures that have a path as an user argument.

# remove leading and trailing double quotes
# windows users can shift + right click a file to copy as path, which has double quotes
csv_name.gsub!('"', '')
DavidGoldwasser commented 3 years ago

@brianlball and @lgentile in addition to writing documentation we should also add a test to this measures. Could try to use FRP for test which might be the quickest, or could make a simple purpose made sample model for the test and documentation. Maybe there is another project we are working on that would be good for test and documentation.

MatthewSteen commented 3 years ago

Another comment/improvement to this measure (and all other measures in general) is to break up the run method into short functions that the run method calls, i.e. apply best coding practices of functions/methods with a single purpose, which would make them more readable, allow for easier debugging, and simplify unit testing.

MatthewSteen commented 3 years ago

Another improvement is to use a standardized time format (ISO 8601) so that users don't have to reformat data after using other tools (e.g. Python Pandas). Maybe that's an EnergyPlus issue though.

Additionally, the measure failed when using the mm/dd/yy format that the arguments suggest because it actually requires mm/dd/yyyy format.

image

kongkun-om commented 1 year ago

Hi, I am interested in using this measure for a time-series calibration. I have monitored data from the building. I tried to find a csv example in the resources folder but it's not available. Could you please advise on the csv formatting? Should I follow ISO 8601 as mentioned above, for example, "07-10 15:00" if "mm:dd hh:mm" is used? Or, will "07/10 15:00" work too? Does the date/time stamp have to be in one column?

If possible, could you please share a csv file example so I can use it as a template?

Thank you.

kongkun-om commented 1 year ago

I encounter these warning and error. Is it because the SQL table does not contain year and second data as shown in the screenshot? Thank you.

[12:57:55.024073 WARN] CSV DateTime 2009-Apr-22 23:00:00 is not in SQL Timeseries Dates [12:57:55.024174 ERROR] no implicit conversion from nil to integer

Capture