lbl-srg / modelica-buildings

Modelica Buildings library
244 stars 153 forks source link

Spawn not recognise the day of week for start day #2926

Open mildwall opened 2 years ago

mildwall commented 2 years ago

Describe the bug I was using the Spawn model Buildings.ThermalZones.EnergyPlus.Examples.SingleFamilyHouse.ShadeControl and the standalone EnergyPlus model EMSWindowShadeControl.idf for a comparison of Spawn against EnergyPlus model with EMS. For this reason, I modified the controllers of the Spawn model to have the same control logic as the model in standalone EnergyPlus, and I also modified the EnergyPlus model to have an idealized cooling system with setpoint temperature of 25 degrees, the same as Spawn model.

While in the comparison, the results of both tools would not really match in some days. For example, the following result is the west zone air temperature in day 70-84. ShadeControl1 It looks like these results don't have the same working/holiday. In the RunPeriod object, the E+ model specified Tuesday as the start day, while Spawn recognise the default start day, Sunday. 20220318233353

After changing the start day in E+ model from Tuesday to Sunday, both results matched each other. ShadeControl2

Suggestion

Version

mwetter commented 2 years ago

@kbenne : As RunPeriod in the idf file is ignored by Spawn, the Day of Week for Start Day is also ignored. As there can be more than one RunPeriod in the idf file, it would not be clear which one to pick. Shall therefore the Day of Week for Start Day be a parameter obtained from Modelica? This way, weekly schedules can be consistent between Modelica and EnergyPlus.

mwetter commented 2 years ago

Solution: Change Modelica so it writes the start day of the run period into ModelicaEnergyPlus.json. Default setting should be Sunday. See https://github.com/NREL/Spawn/blob/a2377839e560034fe03d1d186d334a27a81af138/lib/idfprep.cpp#L38

mwetter commented 2 years ago

Todo:

mwetter commented 2 years ago

@kbenne: Branch issue2926_runPeriod now produces the json file:

{
  "version": "0.2",
  "EnergyPlus": {
    "idf": "/tmp/JModelica.org/jm_tmpex2e6i7p/resources/0/SingleFamilyHouse_TwoSpeed_ZoneAirBalance.idf",
    "weather": "/tmp/JModelica.org/jm_tmpex2e6i7p/resources/1/USA_IL_Chicago-OHare.Intl.AP.725300_TMY3.epw",
    "relativeSurfaceTolerance": 1.00e-06
  },
  "RunPeriod": {
    "day_of_week_for_start_day": "Sunday",
    "apply_weekend_holiday_rule": "No",
    "use_weather_file_daylight_saving_period": "No",
    "use_weather_file_holidays_and_special_days": "No",
    "use_weather_file_rain_indicators": "Yes",
    "use_weather_file_snow_indicators": "Yes"
  },
  "model": {
    "zones": [
      {
        "name": "LIVING ZONE"
      }
    ]
  },
  "fmu": {
      "name": "/mnt/shared/modelica-buildings/spawn-Unconditioned/EnergyPlus.fmu",
      "version": "2.0",
      "kind": "ME"
  }
}

I can change the "Yes" and "No" to boolean if you prefer, but I used the string for now to have the same as in https://github.com/NREL/Spawn/blob/a2377839e560034fe03d1d186d334a27a81af138/lib/idfprep.cpp#L38

Please let me know once you add support for the elements in the RunPeriod entry of the json file. I assume we want them all.

kbenne commented 2 years ago

@mwetter am I correct in assuming that the fields of RunPeriod in the above example json text are not exhaustive? We want to support the other fields too right? I'm especially thinking about the start day and month as that will provide a way to address the long startup times and the related issue https://github.com/lbl-srg/modelica-buildings/issues/2453. I assume that's what you meant by "I assume we want them all." My answer would be yes.

mwetter commented 2 years ago

@kbenne : By "assume we want them all" I meant all of them in the above json snippet.

I did not include start day and month as the FMU will get the start time through the C function call fmi2_import_setup_experiment. Also, if someone generates an FMU, then this FMU may be simulated for different start times. In this case we don't want to have to regenerate the json file as this is already inside the E+ FMU.

See also the instructions in https://github.com/lbl-srg/modelica-buildings/blob/850a10e1cdf1500682293ba64df16288bd873d90/Buildings/ThermalZones/EnergyPlus_9_6_0/Data/RunPeriod.mo#L28 For the E+ integration, you can assume that day_of_week_for_start_day is as if it were in the idf file, the behavior for dayOfWeekIsAtTime0 is handled on my end before I write the json file.

kbenne commented 2 years ago

Indeed my original plan to address #2453 was to use the start time passed in through the setupExperiment api, but then I saw this issue come through and I thought there might be an opportunity to address two things at once while being a bit more transparent. (Yes there are the drawbacks that you mention about the start day/month being baked into the FMU, but I'm starting to believe that might be acceptable.)

The challenge in addressing #2453 is that I need to translate seconds into a day/month (taking into account the year of course). As far as I know EnergyPlus does not natively provide this capability. There is no function to convert elapsed seconds from starting date/time into new date/time b. I'm considering bringing in a date time library to Spawn, but then there is the potential issue that a real date/time library might not match the EnergyPlus calculation. Keep in mind that EnergyPlus time is essentially based on counting steps from the start time in the run period object.

mwetter commented 1 year ago

I don't think this is addressed in the binaries that are on the master, is it?

kbenne commented 1 year ago

Correct. This should still be open. If we keep the changes narrow and focused only on addressing the start day issue according to the solution above, I can take care of this quickly and easily. (e.g. If I don't try to also address #2453).

mwetter commented 6 months ago

The design is that the start day as entered in the .mo file and written to the json file as day_of_week_for_start_day is interpreted as the day of the week at Modelica time=0. Any entry in the idf file that is used to generate the FMU is ignored. There is no support for leap year.

mwetter commented 20 hours ago

This should be addressed in on the master. Next is for LBL to test and close this issue.

mwetter commented 12 hours ago

@kbenne : It looks like EnergyPlus does not correctly process the day of the week. To see it, use branch issue2926_runPeriod, commit 0506fcb8145f698e58c335dbc8f74249a61c3a08 (the head as of now). Run

simulateModel(
  "Buildings.ThermalZones.EnergyPlus_9_6_0.Validation.OutputVariable.OneZoneOneOutputVariable",
  startTime=0,
  stopTime=604800,
  method="Cvode",
  tolerance=1e-06,
  resultFile="OneZoneOneOutputVariable");

and plot image which is correct because the simulation starts on a Sunday and the schedule has electricity use only on Monday to Friday. Now run

simulateModel(
  "Buildings.ThermalZones.EnergyPlus_9_6_0.Validation.OutputVariable.OneZoneOneOutputVariable",
  startTime=86400,
  stopTime=604800,
  method="Cvode",
  tolerance=1e-06,
  resultFile="OneZoneOneOutputVariable");

which starts the simulation one day later. But this gives image showing that EnergyPlus gets the schedule value for Tuesday for the first day of the simulation.

EnergyPlus also writes

86400.000 OneZoneOneOutputVariable.building: Warning from EnergyPlus: RunPeriod: object=SPAWN-RUNPERIOD, start weekday (MONDAY) does not match the start year (2007), corrected to TUESDAY.

It looks like this correction is the cause of the error. The expected behavior is that we have first 5 days with a non-zero schedule value, not 4 days.

The json file has for the first case (starting at t=0) "day_of_week_for_start_day": "Sunday" and for the second case, it has ... "Monday".