Support "Clothing Insulation Calculation Method" for People object #5454
Conversation
🧪 Test Results DashboardSummary
|
| Run | XML File | Status |
|---|---|---|
| run1 | results.xml |
✅ Found |
| run2 | results.xml |
✅ Found |
| run3 | results.xml |
✅ Found |
| const std::string clothingInsulationCalculationMethod = modelObject.clothingInsulationCalculationMethod(); | ||
| idfObject.setString(PeopleFields::ClothingInsulationCalculationMethod, clothingInsulationCalculationMethod); | ||
|
|
||
| if (boost::optional<Schedule> schedule_ = modelObject.clothingInsulationCalculationMethodSchedule()) { | ||
| if (auto idf_schedule_ = translateAndMapModelObject(schedule_.get())) { | ||
| idfObject.setString(PeopleFields::ClothingInsulationCalculationMethodScheduleName, idf_schedule_->nameString()); | ||
| } | ||
| } |
There was a problem hiding this comment.
If I understand correctly what E+ does:
if A10, \field Clothing Insulation Calculation Method = CalculationMethodSchedule, then the clothingInsulationCalculationMethodSchedule is a required object. As such, we should probably handle the case where it's missing in FT, don't you think @joseph-robertson ?
CalculationMethodSchedule
With this choice, the method used can be either the ClothingInsulationSchedule or the DynamicClothingModelASHRAE55, depending on a schedule (to be entered as the next field) that determines which method to use in different time of a day. When this option is chosen, the next field “Clothing Insulation Calculation Method Schedule Name” is a required input.
There was a problem hiding this comment.
Meh, I guess I'm actually not requesting anything here.
It's REQUIRED only if at least one of the PeopleDefinition::ThermalComfortModelType is added. Let's let E+ throw and move on.
| bool People_Impl::setClothingInsulationCalculationMethod(const std::string& clothingInsulationCalculationMethod) { | ||
| const bool result = setString(OS_PeopleFields::ClothingInsulationCalculationMethod, clothingInsulationCalculationMethod); | ||
| if (result) { | ||
| if (istringEqual("ClothingInsulationSchedule", clothingInsulationCalculationMethod)) { | ||
| resetClothingInsulationCalculationMethodSchedule(); | ||
| } else if (istringEqual("DynamicClothingModelASHRAE55", clothingInsulationCalculationMethod)) { | ||
| resetClothingInsulationCalculationMethodSchedule(); | ||
| resetClothingInsulationSchedule(); | ||
| } | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
I am not sure we want to reset the schedules.
If I pick CalculationMethodSchedule then I must provide a ClothingInsulationCalculationMethodSchedule.
That means if I do
people.setClothingInsulationCalculationMethodSchedule(calc_sch)
# Oops, wrong value
people.setClothingInsulationCalculationMethod("DynamicClothingModelASHRAE55")
# Set it again
people.setClothingInsulationCalculationMethod("CalculationMethodSchedule")
# Then I'm still missing the Clo Ins Calc Method Sch...
people.setClothingInsulationCalculationMethodSchedule(calc_sch)
I'm sure we have some other cases where we do something like this in the OS SDK, but I'm not sure we should. Perhaps it's best we handle that in FT?
Another point to make:
- Perhaps setClothingInsulationCalculationMethod("CalculationMethodSchedule") should instantiate the Clothing Insulation Calculation Method Schedule if not there, and set it to 1
- Or perhaps we even make this non optional, and initialize it in the ctor, not sure.
There was a problem hiding this comment.
Yeah, I suppose I agree that setting the calculation method should not reset any fields if you were to accidentally set the wrong calculation method. I'll look into resetting (unused) fields in FT based on the calculation method that is ultimately set.
Yes, since ClothingInsulationCalculationMethodSchedule is required if "CalculationMethodSchedule" is set, we could either default it to 1 in the setter or initialize it in the ctor. At the same time, we could let E+ catch this so the user doesn't run into any unexpected result.
69c5591 to
3d13dd7
Compare
This looks like an actual test failure that must be addressed. |
|
CI Results for add2e94:
|
Co-authored-by: Joe Robertson <joseph.robertson@nrel.gov> Co-authored-by: Julien Marrec <julien.marrec@gmail.com>
``` CMake Warning (dev) at CMakeLists.txt:1033 (elseif): ELSEIF called with no arguments, it will be skipped. This warning is for project developers. Use -Wno-dev to suppress it. ```
|
The branch history is too messed up and there are conflicts, going to squash commits then rebase onto develop. |
add2e94 to
c30f6f2
Compare
Pull request overview
In FT, set schedules using values of Clothing Insulation Calculation Method:
Pull Request Author
src/model/test)src/energyplus/Test)src/osversion/VersionTranslator.cpp)Labels:
IDDChangeAPIChangePull Request - Ready for CIso that CI builds your PRReview Checklist
This will not be exhaustively relevant to every PR.