-
Notifications
You must be signed in to change notification settings - Fork 190
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
Add Utility for Generating Alfalfa Metadata to OpenStudio #5236
base: develop
Are you sure you want to change the base?
Conversation
@kbenne what do you think about the language of Right now all the |
@kbenne this is pretty much there.
|
@TShapinsky as we discussed, it would be a good idea to test a workflow that excerses this new feature. You can see an example of workflow tests here. Note they are not Google Tests, but instead these are implemented purely in the CTest wrapper. |
I created a new issue to create documentation for this new API. |
…functions to make simpler interface
@kbenne I'm happy where everything is, with the exception of the behaviour for dealing with duplicate ids. One possible approach is to modify duplicate ids on export so they become unique. This would work, but it could be confusing and it is also non-deterministic. If someone was expecting a certain set of IDs from a measure they might not get them if a previous measure adds points of the same id. Another approach is to throw an error on export which could be annoying. It also runs the possibility of weird behaviour where based on naming of model objects certain models would error while others didn't. Are names unique across an IDF? or are they just unique to an idd type? A possible third approach would be to refactor the id out of the point and have it be set in the AlfalfaJSON. This is probably the most correct as it is a key used to refer to the point and thus not something that the point needs to know about. This would allow for runtime checking of point Id against the existing points. I think the best option may be a combination of the first and third. Refactoring the point id to be stored in the AlfalfaJSON object and dynamically renaming point ids to prevent overlap. A warning could also be printed to alert of this happening. But that would only be shown locally, so it wouldn't include weird behaviour that happens when the measures interact with the baked in alfalfa measures. Maybe after this we need to reevaluate if we want to be automatically baking points into alfalfa or if we should just provide the measures we create for generating generic points as a resource for people putting together workflows. That would allow for the maximum amount of reproducibility between alfalfa and local development. |
@jmarrec I would grateful to have you weigh in on this. The challenge is that I'm not sure that you have enough context or any available time. If you don't have time, that's understandable, just say so. My intention is to merge this in time for the next release unless there are any serious red flags that emerge. |
@wenyikuang can you investigate the CI failures? |
Looks the windows-incremental is lost connection from the jenkins, let me look |
If i remember correctly, the osx build fails were from tests failing for BCL measures. |
alfalfa.exposeConstant(10, "safe value") | ||
alfalfa.exposeMeter("Facility:Electricity", "Facility Electricity") | ||
alfalfa.exposeActuator("somehting", "another thing", "key", "example actuator") | ||
alfalfa.exposeOutputVariable("Whole Building", "Facility Total Purchased Electricity Energy", "output variable") |
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.
Is this a change? I seem to recall that these originally took ModelObject instances. I think when we @TShapinsky last spoke you were contemplating a change.
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.
A risk I can think of is that the final names might not be known at ModelMeasure time.
alfalfa.exposeMeter("Electricity:Facility", "Electricity Meter String:EPlus:Python") | ||
|
||
meter_object = openstudio.IdfObject.load("Output:Meter, Electricity:Facility;").get() | ||
alfalfa.exposeFromObject(meter_object, "Electricity Meter IDF:Eplus:Python") |
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.
Does this method work with a ModelObject ?
meter_object = openstudio.model.OutputMeter(model) | ||
meter_object.setFuelType(openstudio.FuelType("Electricity")) | ||
meter_object.setInstallLocationType(openstudio.InstallLocationType("Facility")) | ||
alfalfa.exposeFromObject(meter_object, "Electricity Meter OSM:Model:Python") |
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 see. Questions answered.
@@ -125,6 +128,24 @@ set(filetypes_src | |||
filetypes/WorkflowStepResult.cpp | |||
) | |||
|
|||
set(alfalfa_src |
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.
Perhaps src/utilities/alfalfa should come up a level as utilities is taking on a lot. Perhaps src/alfalfa?
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.
That's what i originally had, but then i got humble. I'm happy to move it back up to that level again. It would also make some of the ruby bindings a bit easier.
Resolve the issue of connection between jenkins and windows-incre-on-aws (From another PR but using the same node) Or are you mentioning these one?
|
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.
This is good work. I'm just generally picky.
There are a couple of constructs I'd change, I'd also suggest taking your files for a spin with clang-tidy to modernize the code a bit and follow best practices.
My main point is more around the articulation of the classes between themselves, and what public API should be like (probably use actual type values to store stuff and not JSON, provide getters, some methods should probably be private, and error handling should be reworked a bit).
alfalfa.exposeActuator("somehting", "another thing", "key", "example actuator") | ||
alfalfa.exposeOutputVariable("Whole Building", "Facility Total Purchased Electricity Energy", "output variable") | ||
alfalfa.exposeGlobalVariable("global_1", "global variable") | ||
super().run(model, runner, user_arguments) # Do **NOT** remove this line |
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 find the placement of this call to super() quite strange. Is there a reason it's done after exposing the alfalfa things?
If not, I'd keep it first in the routine, so people are less likely to remove it when they copy your measure.
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.
There is not a reason, I'll fix that in the next commit.
openstudio::measure::OSRunner runner(workflow); | ||
|
||
openstudio::measure::OSArgumentMap arguments; | ||
measurePtr->run(model, runner, arguments); |
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.
Is there anything you can test here?!
Make sure your alfalfa.exposeXXX calls actually did something
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.
Check the runner's alfalfa has points at least.
@@ -28,6 +28,7 @@ | |||
from . import openstudioosversion as osversion | |||
from . import openstudioradiance as radiance | |||
from . import openstudiosdd as sdd | |||
from . import openstudioutilitiesalfalfa as alfalfa |
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.
This doesn't follow the other utilities convention. Maybe I'll discover a good reason deeper in the review.
user_arguments: openstudio.measure.OSArgumentMap, | ||
): | ||
"""Defines what happens when the measure is run.""" | ||
super().run(workspace, runner, user_arguments) # Do **NOT** remove this line |
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.
Ok this one is first in line and has the comment
meter_object = openstudio.model.OutputMeter(model) | ||
meter_object.setFuelType(openstudio.FuelType("Electricity")) | ||
meter_object.setInstallLocationType(openstudio.InstallLocationType("Facility")) | ||
alfalfa.exposeFromObject(meter_object, "Electricity Meter OSM:Model:Python") |
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.
Note to self to check what the second param is. Edit: displayName
m_impl->exposePoint(point); | ||
} | ||
|
||
boost::optional<AlfalfaPoint> AlfalfaJSON::exposeFromObject(const openstudio::IdfObject& idf_object, const std::string& display_name) { |
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.
THe vast majority of the code I've read was "throwy", but here you go with LOG and return boost::none. That's fine, just noting.
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.
The general pattern was I log and return boost::none
everywhere possible, but in ctors you can't do that (to my knowledge) to I throw a std::runtime_error
.
boost::optional<AlfalfaPoint> AlfalfaJSON::exposeFromComponent(const AlfalfaComponent& component, const std::string& display_name) { | ||
std::string _display_name = display_name; | ||
if (display_name.size() == 0) { | ||
if (component.type == "Actuator") { | ||
_display_name = "Actuator for " + component.parameters["component_name"].asString() + ":" + component.parameters["component_type"].asString() | ||
+ ":" + component.parameters["control_type"].asString(); | ||
} else if (component.type == "Constant") { | ||
LOG(Error, "Constant points must be provided with a display name"); | ||
return boost::none; |
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.
There's something bothering me with error handling here, or maybe public-ness of the exposeFromComponent method.
Why ever allow a component.type Constant to NOT have a displayName to begin with?!
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.
It was mostly in the interest of making the base programming interface as simple as possible for users to get up and running with. Maybe I went too far.
/** | ||
* Get a vector of all points currently exported to the Alfalfa API | ||
*/ | ||
std::vector<AlfalfaPoint> getPoints(); |
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 think points()
better matches our general convention.
@@ -18,6 +18,13 @@ void OSWorkflow::runPostProcess() { | |||
openstudio::workflow::util::gatherReports(workflowJSON.absoluteRunDir(), workflowJSON.absoluteRootDir()); | |||
LOG(Info, "Finished gathering reports"); | |||
} | |||
// If no points have been exported, skip file creation | |||
if (runner.alfalfa().getPoints().size() > 0) { |
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.
std::vector::empty()
@wenyikuang cppcheck is broken, you should fix that: https://github.com/NREL/OpenStudio/actions/runs/10853524579/job/30432985755#step:3:16 |
protected: | ||
friend class openstudio::alfalfa::AlfalfaPoint; |
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.
All members are protected which I don't think is necessary, should be private.
That friend decl is weird, and is only needed because you access these members without going to an actual function. Again, is the pImpl needed? If so, make it a proper class with getters and setters.
|
[Developer] Adjust alfalfa_utility
CI Results for 2e67d5c:
|
Alfalfa is a platform which allows users to interact with OpenStudio/EnergyPlus models in real time. In order to accomplish this users must specify what components of the model to connect with Alfalfa as input and output points. This PR moves the mechanisms for creating "Alfalfa Points" into the OpenStudio runner so that they may be accessed during the measure workflow.
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.