Fix #4439 - Correctly Reverse Translate gbXML Schedules #4440

jmarrec · 2021-09-21T08:27:53Z

Pull request overview

Fix #4439 - Correctly Reverse Translate gbXML Schedules.

This is largely a PR that adds tests for gbxml Schedules in the RT. The fix is a single line change.

Pull Request Author

gbXML ReverseTranslator API Changes / Additions
gbXML ReverseTranslator Tests (in src/gbxml/Test)
All new and existing tests passes
If methods have been deprecated, update rest of code to use the new methods

Labels:

If change to an IDD file, add the label IDDChange
If breaking existing API, add the label APIChange
If deemed ready, add label Pull Request - Ready for CI so that CI builds your PR

Review Checklist

This will not be exhaustively relevant to every PR.

Perform a Code Review on GitHub
Code Style, strip trailing whitespace, etc.
All related changes have been implemented: model changes, model tests, FT changes, FT tests, VersionTranslation, OS App
Labeling is ok
If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
If feature, test running new feature, try creative ways to break it
CI status: all green or justified

we should have proper checking and error handling

jmarrec · 2021-09-21T08:32:12Z

resources/gbxml/TestSchedules.xml

+  <Schedule type="Fraction" id="Simple_Schedule">
+    <YearSchedule id="Simple_Schedule_YearSchedule">
+      <BeginDate>2017-01-01</BeginDate>
+      <EndDate>2017-12-31</EndDate>
+      <WeekScheduleId weekScheduleIdRef="Simple_Schedule_WeekSchedule" />
+    </YearSchedule>
+    <Name>Simple Year Schedule</Name>
+  </Schedule>


A simple schedule

jmarrec · 2021-09-21T08:33:10Z

resources/gbxml/TestSchedules.xml

+      <WeekScheduleId weekScheduleIdRef="Complex_Schedule_WeekSchedule_BeginYear" />
+    </YearSchedule>
+    <YearSchedule id="Complex_Schedule_YearSchedule_EndYear">
+      <BeginDate>2017-07-01</BeginDate>
+      <EndDate>2017-12-31</EndDate>
+      <WeekScheduleId weekScheduleIdRef="Complex_Schedule_WeekSchedule_EndYear" />
+    </YearSchedule>
+    <Name>Complex Year Schedule</Name>
+  </Schedule>
+  <WeekSchedule type="Fraction" id="Complex_Schedule_WeekSchedule_BeginYear">
+    <Day dayScheduleIdRef="Complex_Schedule_DaySchedule_BeginYear" dayType="All" />
+    <Name>Complex Year Schedule - Typical Week January to June</Name>
+  </WeekSchedule>
+  <DaySchedule type="Fraction" id="Complex_Schedule_DaySchedule_BeginYear">
+    <ScheduleValue>0</ScheduleValue>
+    <ScheduleValue>0</ScheduleValue>
+    <ScheduleValue>0</ScheduleValue>
+    <ScheduleValue>0</ScheduleValue>
+    <ScheduleValue>0</ScheduleValue>
+    <ScheduleValue>0</ScheduleValue>
+    <ScheduleValue>0.1</ScheduleValue>
+    <ScheduleValue>0.3</ScheduleValue>
+    <ScheduleValue>0.9</ScheduleValue>
+    <ScheduleValue>0.9</ScheduleValue>
+    <ScheduleValue>0.9</ScheduleValue>
+    <ScheduleValue>0.9</ScheduleValue>
+    <ScheduleValue>0.8</ScheduleValue>
+    <ScheduleValue>0.9</ScheduleValue>
+    <ScheduleValue>0.9</ScheduleValue>
+    <ScheduleValue>0.9</ScheduleValue>
+    <ScheduleValue>0.9</ScheduleValue>
+    <ScheduleValue>0.5</ScheduleValue>
+    <ScheduleValue>0.3</ScheduleValue>
+    <ScheduleValue>0.3</ScheduleValue>
+    <ScheduleValue>0.2</ScheduleValue>
+    <ScheduleValue>0.2</ScheduleValue>
+    <ScheduleValue>0</ScheduleValue>
+    <ScheduleValue>0</ScheduleValue>
+    <Name>Complex Year Schedule - Typical Day January to June</Name>
+  </DaySchedule>
+
+  <WeekSchedule type="Fraction" id="Complex_Schedule_WeekSchedule_EndYear">
+    <Day dayScheduleIdRef="Complex_Schedule_DaySchedule_EndYear_Weekday" dayType="Weekday" />
+    <Day dayScheduleIdRef="Complex_Schedule_DaySchedule_EndYear_Weekend" dayType="Weekend" />
+    <Name>Complex Year Schedule - Typical Week July to December</Name>
+  </WeekSchedule>
+  <DaySchedule type="Fraction" id="Complex_Schedule_DaySchedule_EndYear_Weekday">
+    <ScheduleValue>0</ScheduleValue>
+    <ScheduleValue>0</ScheduleValue>
+    <ScheduleValue>0</ScheduleValue>
+    <ScheduleValue>0</ScheduleValue>
+    <ScheduleValue>0</ScheduleValue>
+    <ScheduleValue>0</ScheduleValue>
+    <ScheduleValue>0.5</ScheduleValue>
+    <ScheduleValue>0.5</ScheduleValue>
+    <ScheduleValue>0.5</ScheduleValue>
+    <ScheduleValue>0.5</ScheduleValue>
+    <ScheduleValue>0.5</ScheduleValue>
+    <ScheduleValue>0.5</ScheduleValue>
+    <ScheduleValue>0.5</ScheduleValue>
+    <ScheduleValue>0.5</ScheduleValue>
+    <ScheduleValue>0.5</ScheduleValue>
+    <ScheduleValue>0.5</ScheduleValue>
+    <ScheduleValue>0.5</ScheduleValue>
+    <ScheduleValue>0.5</ScheduleValue>
+    <ScheduleValue>0.5</ScheduleValue>
+    <ScheduleValue>0.5</ScheduleValue>
+    <ScheduleValue>0.5</ScheduleValue>
+    <ScheduleValue>0.5</ScheduleValue>
+    <ScheduleValue>0</ScheduleValue>
+    <ScheduleValue>0</ScheduleValue>
+    <Name>Complex Year Schedule - Typical Weekday July to December</Name>
+  </DaySchedule>
+  <DaySchedule type="Fraction" id="Complex_Schedule_DaySchedule_EndYear_Weekend">
+    <ScheduleValue>0</ScheduleValue>
+    <ScheduleValue>0</ScheduleValue>
+    <ScheduleValue>0</ScheduleValue>
+    <ScheduleValue>0</ScheduleValue>
+    <ScheduleValue>0</ScheduleValue>
+    <ScheduleValue>0</ScheduleValue>
+    <ScheduleValue>1.0</ScheduleValue>
+    <ScheduleValue>1.0</ScheduleValue>
+    <ScheduleValue>1.0</ScheduleValue>
+    <ScheduleValue>1.0</ScheduleValue>
+    <ScheduleValue>1.0</ScheduleValue>
+    <ScheduleValue>1.0</ScheduleValue>
+    <ScheduleValue>1.0</ScheduleValue>
+    <ScheduleValue>1.0</ScheduleValue>
+    <ScheduleValue>1.0</ScheduleValue>
+    <ScheduleValue>1.0</ScheduleValue>
+    <ScheduleValue>1.0</ScheduleValue>
+    <ScheduleValue>1.0</ScheduleValue>
+    <ScheduleValue>1.0</ScheduleValue>
+    <ScheduleValue>1.0</ScheduleValue>
+    <ScheduleValue>1.0</ScheduleValue>
+    <ScheduleValue>1.0</ScheduleValue>
+    <ScheduleValue>0</ScheduleValue>
+    <ScheduleValue>0</ScheduleValue>
+    <Name>Complex Year Schedule - Typical Weekend July to December</Name>
+  </DaySchedule>


A more complex schedule: two separate periods

Jan to June, same day for all

July to December, one day schedule for weekday and one for weekends

jmarrec · 2021-09-21T08:33:29Z

src/gbxml/Test/ReverseTranslator_GTest.cpp

+    EXPECT_EQ(schDay, schWeek.tuesdaySchedule().get());
+    ASSERT_TRUE(schWeek.wednesdaySchedule());
+    EXPECT_EQ(schDay, schWeek.wednesdaySchedule().get());
+    ASSERT_TRUE(schWeek.thursdaySchedule());
+    EXPECT_EQ(schDay, schWeek.thursdaySchedule().get());
+    ASSERT_TRUE(schWeek.fridaySchedule());
+    EXPECT_EQ(schDay, schWeek.fridaySchedule().get());
+    ASSERT_TRUE(schWeek.saturdaySchedule());
+    EXPECT_EQ(schDay, schWeek.saturdaySchedule().get());
+    ASSERT_TRUE(schWeek.holidaySchedule());
+    EXPECT_EQ(schDay, schWeek.holidaySchedule().get());
+    ASSERT_TRUE(schWeek.summerDesignDaySchedule());
+    EXPECT_EQ(schDay, schWeek.summerDesignDaySchedule().get());
+    ASSERT_TRUE(schWeek.winterDesignDaySchedule());
+    EXPECT_EQ(schDay, schWeek.winterDesignDaySchedule().get());
+    ASSERT_TRUE(schWeek.customDay1Schedule());
+    EXPECT_EQ(schDay, schWeek.customDay1Schedule().get());
+    ASSERT_TRUE(schWeek.customDay2Schedule());
+    EXPECT_EQ(schDay, schWeek.customDay2Schedule().get());
+  }
+
+  // Test Complex Schedule
+  {
+    auto schYear_ = m.getConcreteModelObjectByName<ScheduleYear>("Complex Year Schedule");
+    EXPECT_TRUE(schYear_);
+    ASSERT_EQ(2U, schYear_->dates().size());
+    EXPECT_EQ(june30, schYear_->dates()[0]);
+    EXPECT_EQ(dec31, schYear_->dates()[1]);
+    ASSERT_EQ(2U, schYear_->scheduleWeeks().size());
+
+    {
+      auto schWeek = schYear_->scheduleWeeks()[0];
+      EXPECT_EQ("Complex Year Schedule - Typical Week January to June", schWeek.nameString());
+
+      auto schDay_ = m.getConcreteModelObjectByName<ScheduleDay>("Complex Year Schedule - Typical Day January to June");
+      ASSERT_TRUE(schDay_);
+      auto schDay = schDay_.get();
+
+      ASSERT_TRUE(schWeek.sundaySchedule());
+      EXPECT_EQ(schDay, schWeek.sundaySchedule().get());
+      ASSERT_TRUE(schWeek.mondaySchedule());
+      EXPECT_EQ(schDay, schWeek.mondaySchedule().get());
+      ASSERT_TRUE(schWeek.tuesdaySchedule());
+      EXPECT_EQ(schDay, schWeek.tuesdaySchedule().get());
+      ASSERT_TRUE(schWeek.wednesdaySchedule());
+      EXPECT_EQ(schDay, schWeek.wednesdaySchedule().get());
+      ASSERT_TRUE(schWeek.thursdaySchedule());
+      EXPECT_EQ(schDay, schWeek.thursdaySchedule().get());
+      ASSERT_TRUE(schWeek.fridaySchedule());
+      EXPECT_EQ(schDay, schWeek.fridaySchedule().get());
+      ASSERT_TRUE(schWeek.saturdaySchedule());
+      EXPECT_EQ(schDay, schWeek.saturdaySchedule().get());
+      ASSERT_TRUE(schWeek.holidaySchedule());
+      EXPECT_EQ(schDay, schWeek.holidaySchedule().get());
+      ASSERT_TRUE(schWeek.summerDesignDaySchedule());
+      EXPECT_EQ(schDay, schWeek.summerDesignDaySchedule().get());
+      ASSERT_TRUE(schWeek.winterDesignDaySchedule());
+      EXPECT_EQ(schDay, schWeek.winterDesignDaySchedule().get());
+      ASSERT_TRUE(schWeek.customDay1Schedule());
+      EXPECT_EQ(schDay, schWeek.customDay1Schedule().get());
+      ASSERT_TRUE(schWeek.customDay2Schedule());
+      EXPECT_EQ(schDay, schWeek.customDay2Schedule().get());
+    }
+
+    {
+      auto schWeek = schYear_->scheduleWeeks()[1];
+      EXPECT_EQ("Complex Year Schedule - Typical Week July to December", schWeek.nameString());
+
+      auto schDayWeekday_ = m.getConcreteModelObjectByName<ScheduleDay>("Complex Year Schedule - Typical Weekday July to December");
+      ASSERT_TRUE(schDayWeekday_);
+      auto schDayWeekday = schDayWeekday_.get();
+
+      auto schDayWeekend_ = m.getConcreteModelObjectByName<ScheduleDay>("Complex Year Schedule - Typical Weekend July to December");
+      ASSERT_TRUE(schDayWeekend_);
+      auto schDayWeekend = schDayWeekend_.get();
+
+      ASSERT_TRUE(schWeek.mondaySchedule());
+      EXPECT_EQ(schDayWeekday, schWeek.mondaySchedule().get());
+      ASSERT_TRUE(schWeek.tuesdaySchedule());
+      EXPECT_EQ(schDayWeekday, schWeek.tuesdaySchedule().get());
+      ASSERT_TRUE(schWeek.wednesdaySchedule());
+      EXPECT_EQ(schDayWeekday, schWeek.wednesdaySchedule().get());
+      ASSERT_TRUE(schWeek.thursdaySchedule());
+      EXPECT_EQ(schDayWeekday, schWeek.thursdaySchedule().get());
+      ASSERT_TRUE(schWeek.fridaySchedule());
+      EXPECT_EQ(schDayWeekday, schWeek.fridaySchedule().get());
+
+      ASSERT_TRUE(schWeek.saturdaySchedule());
+      EXPECT_EQ(schDayWeekend, schWeek.saturdaySchedule().get());
+      ASSERT_TRUE(schWeek.sundaySchedule());
+      EXPECT_EQ(schDayWeekend, schWeek.sundaySchedule().get());
+
+      EXPECT_FALSE(schWeek.holidaySchedule());
+      EXPECT_FALSE(schWeek.summerDesignDaySchedule());
+      EXPECT_FALSE(schWeek.winterDesignDaySchedule());
+      EXPECT_FALSE(schWeek.customDay1Schedule());
+      EXPECT_FALSE(schWeek.customDay2Schedule());
+    }
+  }
+}


Test that everything works as expected.

jmarrec · 2021-09-21T08:35:27Z

src/gbxml/MapSchedules.cpp

@@ -218,7 +218,7 @@ namespace gbxml {
      OS_ASSERT(yd.calendarYear().get() == std::stoi(endDateParts.at(0)));
      openstudio::Date endDate = yd.makeDate(std::stoi(endDateParts.at(1)), std::stoi(endDateParts.at(2)));

-      std::string weekScheduleId = element.child("WeekScheduleId").attribute("weekScheduleIdRef").value();
+      std::string weekScheduleId = scheduleYearElement.child("WeekScheduleId").attribute("weekScheduleIdRef").value();


Actual fix...

Really we need error handling in gbxml... There is currently no validation against the gbXML schema happening and there is very little checking that is done manually (like ensuring that required values/attributes are supplied, or checking that a YearSchedule references a weekScheduleIdRef you make sure it can be found)

I'll open a separate enhancement request for that.

indeed, it's worth discussing adding in some logic or external lib that can validate gbXML. Thanks for opening up an enhancement request.

ci-commercialbuildings · 2021-09-21T09:10:54Z

CI Results for d08d7a9:

jmarrec · 2021-09-21T09:24:24Z

@tijcolem I think we have an issue with the way cbci_jenkins_lib is supposed to find and replace the post for CI results :)

jmarrec · 2021-09-21T09:36:59Z

Ok so demonstrated that tests fail before fix: https://ci.commercialbuildings.dev/blue/organizations/jenkins/openstudio-incremental-osx/detail/PR-4440/1/pipeline#step-63-log-7

And work with fix.

tijcolem · 2021-09-21T17:29:44Z

@jmarrec made a few changes to the comment block find/replace. Seems to be working on another PR right now so hopefully next go around for you it'll post correctly.

tijcolem

looks good and CI is happy!

jmarrec added 3 commits September 21, 2021 10:25

Add a basic test for #4439 (gbxml RT for schedules)

04652b8

Add a more complex test for #4439 - RT gbxml schedule

b92bb16

Minimal fix for #4439 - Correctly RT gbXML Schedules

d08d7a9

we should have proper checking and error handling

jmarrec added severity - Normal Bug component - gbXML Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. labels Sep 21, 2021

jmarrec requested a review from tijcolem September 21, 2021 08:27

jmarrec self-assigned this Sep 21, 2021

jmarrec commented Sep 21, 2021

View reviewed changes

jmarrec mentioned this pull request Sep 21, 2021

GbXML ReverseTranslator needs validation and/or better error handling #4441

Closed

jmarrec added the Ready for Merge Set this once CI is passing and reviews are approved. label Sep 21, 2021

tijcolem approved these changes Sep 21, 2021

View reviewed changes

tijcolem merged commit c4a5d4f into develop Sep 21, 2021

tijcolem deleted the 4439_gbXML_RT_Schedules branch September 21, 2021 17:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4439 - Correctly Reverse Translate gbXML Schedules #4440

Fix #4439 - Correctly Reverse Translate gbXML Schedules #4440

jmarrec commented Sep 21, 2021 •

edited

jmarrec Sep 21, 2021

jmarrec Sep 21, 2021

jmarrec Sep 21, 2021

jmarrec Sep 21, 2021

tijcolem Sep 21, 2021

ci-commercialbuildings commented Sep 21, 2021 •

edited

jmarrec commented Sep 21, 2021

jmarrec commented Sep 21, 2021

tijcolem commented Sep 21, 2021 •

edited

tijcolem left a comment

Fix #4439 - Correctly Reverse Translate gbXML Schedules #4440

Fix #4439 - Correctly Reverse Translate gbXML Schedules #4440

Conversation

jmarrec commented Sep 21, 2021 • edited

Pull request overview

Pull Request Author

Review Checklist

jmarrec Sep 21, 2021

Choose a reason for hiding this comment

jmarrec Sep 21, 2021

Choose a reason for hiding this comment

jmarrec Sep 21, 2021

Choose a reason for hiding this comment

jmarrec Sep 21, 2021

Choose a reason for hiding this comment

tijcolem Sep 21, 2021

Choose a reason for hiding this comment

ci-commercialbuildings commented Sep 21, 2021 • edited

jmarrec commented Sep 21, 2021

jmarrec commented Sep 21, 2021

tijcolem commented Sep 21, 2021 • edited

tijcolem left a comment

Choose a reason for hiding this comment

jmarrec commented Sep 21, 2021 •

edited

ci-commercialbuildings commented Sep 21, 2021 •

edited

tijcolem commented Sep 21, 2021 •

edited