-
Notifications
You must be signed in to change notification settings - Fork 193
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.
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
Improve weather file handling in OS Workflow #5011
Conversation
Now we make sure to look in the Model for the final weather file defined after all Measures have run. We also still iniitialize the weather file from the OSW if one is defined. close #5009
Here is a workflow that demonstrates the issue. |
src/workflow/RunTranslator.cpp
Outdated
@@ -38,6 +38,8 @@ void OSWorkflow::runTranslator() { | |||
auto runDir = workflowJSON.absoluteRunDir(); | |||
OS_ASSERT(openstudio::filesystem::is_directory(runDir)); | |||
|
|||
applyWeatherFileFromModel(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Workflow gem seem to be doing it in APplyMeasure when it's a ModelMeasure: https://github.com/NREL/OpenStudio-workflow-gem/blob/696306b5b24cd597edc6ab04faa1e618f99db190/lib/openstudio/workflow/util/measure.rb#L551-L571
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you actually need to complete this unfinished block instead:
OpenStudio/src/workflow/ApplyMeasure.cpp
Lines 215 to 224 in 4d820d0
if (measureType == MeasureType::ModelMeasure) { | |
if (auto weatherFile_ = model.weatherFile()) { | |
if (auto p_ = weatherFile_->path()) { | |
// Probably a workflowJSON.findFile() call... | |
// m_epwPath_ = p_; | |
} else { | |
LOG(Warn, "Weather file object found in model but no path is given"); | |
} | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC this unfinished block is really where it needs to happen, so any downstream OpenStudio ModelMeasure that needs to use the workflow path via runner.lastEpwFile or whatever will get the right info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I forgot about Runner::lastEpwFile. But I think it still needs to happen before translation too, because don't we need to account for the fact that the last ModelMeasure could have changed the weather location?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating last runner epwfile is done prior to measure run currently. The unfinished code block I link to is supposed to be executed after the measure run.
I'm not against changing what workflow-gem does, but it's 1) easier to Match and 2) clearer and probably 3) less error prone
Now we make sure to look in the Model for the final weather file defined after all Measures have run.
We also still iniitialize the weather file from the OSW if one is defined.
close #5009
Pull request overview
Pull Request Author
src/model/test
)src/energyplus/Test
)src/osversion/VersionTranslator.cpp
)Labels:
IDDChange
APIChange
Pull Request - Ready for CI
so that CI builds your PRReview Checklist
This will not be exhaustively relevant to every PR.