Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add ability to create Holiday Schedules in ScheduleRuleset #3814

Merged
merged 18 commits into from
Jan 21, 2020
Merged

Add ability to create Holiday Schedules in ScheduleRuleset #3814

merged 18 commits into from
Jan 21, 2020

Conversation

jmarrec
Copy link
Collaborator

@jmarrec jmarrec commented Dec 11, 2019 •
edited
Loading

Pull request overview

See #3783 for the rationale used, explaining why I chose to add the Holiday schedule to ScheduleRuleset instead of ScheduleRule::applyToHoliday (TL;DR: if in ScheduleRule, potentially multiple ScheduleRules with applyToHoliday=true could be applied during the same week (this translate to a Schedule:Week:Daily...))

Pull Request Author

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

  • Model API Changes / Additions
  • Any new or modified fields have been implemented in the EnergyPlus ForwardTranslator (and ReverseTranslator as appropriate)
  • Model API methods are tested (in src/model/test)
  • EnergyPlus ForwardTranslator Tests (in src/energyplus/Test)
  • If a new object or method, added a test in NREL/OpenStudio-resources: Add Link
  • If needed, added VersionTranslation rules for the objects (src/osversion/VersionTranslator.cpp)
  • Checked behavior in OpenStudioApplication, adjusted policies as needed (src/openstudio_lib/library/OpenStudioPolicy.xml)
  • Verified that C# bindings built fine on Windows, partial classes used as needed, etc.
  • All new and existing tests passes
  • If methods have been deprecated, update rest of code to use the new methods: N/A

Labels:

  • If change to an IDD file, add the label IDDChange
  • If breaking existing API, add the label APIChange: N/A
  • 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

@jmarrec jmarrec added component - Model Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. Develop3 IDDChange labels Dec 11, 2019
@jmarrec jmarrec self-assigned this Dec 11, 2019
@jmarrec jmarrec mentioned this pull request Dec 11, 2019
Merged
@joseph-robertson
Copy link
Collaborator

In general I think this looks good. I need to wait on receiving some example idf file(s) to see if this implementation will accommodate the needs.

jmarrec
jmarrec commented Dec 12, 2019
View reviewed changes
resources/model/OpenStudio.idd
\type choice
\default No
\key Yes
\key No
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this (currently unused) field from Schedule:Rule. Handled in Version Translation.

resources/model/OpenStudio.idd
A6, \field Winter Design Day Schedule Name
\type object-list
\object-list DayScheduleNames
A7; \field Holiday Schedule Name
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New field on Schedule:Ruleset

src/energyplus/ForwardTranslator.cpp
@@ -499,6 +499,7 @@ Workspace ForwardTranslator::translateModelPrivate( model::Model & model, bool f
}


// TODO: is it time to uncomment that?
// temp code
if (!m_keepRunControlSpecialDays){
// DLM: we will not translate these objects until we support holidays in the GUI
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's time to uncomment that.

src/energyplus/ForwardTranslator/ForwardTranslateScheduleRuleset.cpp Outdated
@@ -148,6 +148,8 @@ boost::optional<IdfObject> ForwardTranslator::translateScheduleRuleset( Schedule
ScheduleDay defaultDaySchedule = modelObject.defaultDaySchedule();
ScheduleDay summerDesignDaySchedule = modelObject.summerDesignDaySchedule();
ScheduleDay winterDesignDaySchedule = modelObject.winterDesignDaySchedule();
// ScheduleRuleset is the one carrying the Holiday Schedule
ScheduleDay holidayDaySchedule = modelObject.holidaySchedule();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would return the scheduleRuleset.defaultDaySchedule() if not explicitly set (=same behavior as before). The translation of this ScheduleDay is called later

src/energyplus/ForwardTranslator/ForwardTranslateScheduleRuleset.cpp Outdated

translateAndMapModelObject(defaultDaySchedule);
translateAndMapModelObject(summerDesignDaySchedule);
translateAndMapModelObject(winterDesignDaySchedule);
translateAndMapModelObject(holidayDaySchedule);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Translation of this potentially new ScheduleDay to handle

src/model/ScheduleRule.cpp
OS_ASSERT(value);
return openstudio::istringEqual(value.get(), "Yes");
}
*/
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleting all already-commented-out regarding Apply Holiday in Schedule:Rule now that the IDD doesn't bear the field => No API change

src/model/ScheduleRuleset.cpp
test = newScheduleRuleset.setPointer(OS_Schedule_RulesetFields::HolidayScheduleName,newHolidaySchedule.handle());
OS_ASSERT(test);
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All additions to this file are basically copied over from Winter/Summer Design Day, since the Holiday Schedule will behave exactly like them: they all are "Special Day Profiles"

src/osversion/VersionTranslator.cpp

// Note: OS:ScheduleRuleset got a new optional field at the end, so no-op
// } else if (iddname == "OS:Schedule:Ruleset") {

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Version Translation for IDD Changes

src/energyplus/ForwardTranslator/ForwardTranslateScheduleRuleset.cpp Outdated Show resolved Hide resolved
src/energyplus/ForwardTranslator/ForwardTranslateScheduleRuleset.cpp Outdated Show resolved Hide resolved
@tijcolem tijcolem closed this Dec 13, 2019
@tijcolem tijcolem reopened this Dec 13, 2019
@tijcolem tijcolem changed the base branch from develop3 to develop December 13, 2019 18:33
@tijcolem tijcolem removed the Develop3 label Dec 13, 2019
@jmarrec
Copy link
Collaborator Author

jmarrec commented Jan 16, 2020 •
edited
Loading

I need to move the VT into the right block. Seems like in merging latest develop it went wrong (originally it was in update_2_9_0_to_3_0_0)

@kbenne kbenne merged commit 0ae5947 into develop Jan 21, 2020
@jmarrec jmarrec deleted the 3783_ScheduleHolidays branch January 22, 2020 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joseph-robertson joseph-robertson joseph-robertson left review comments

Assignees

@jmarrec jmarrec

Labels
component - Model IDDChange Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support Holidays in OpenStudio SDK
4 participants
@jmarrec @joseph-robertson @kbenne @tijcolem