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.

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

Open
wants to merge 39 commits into
base: develop
Choose a base branch
from

Conversation

kbenne
Copy link
Contributor

@kbenne kbenne commented Aug 16, 2024

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

  • 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)
  • 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

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

@kbenne kbenne self-assigned this Aug 16, 2024
@TShapinsky TShapinsky changed the title Basic structure for Alfalfa OpenStudio integration Add Utility for Generating Alfalfa Metadata to OpenStudio Aug 16, 2024
@TShapinsky
Copy link
Member

@kbenne what do you think about the language of add vs something like export.

Right now all the AlfalfaJSON methods are phrased as add like addGlobalVariable, addMeter, etc. I wonder if it might make more sense to replace add with export since what you're doing is exporting a GlobalVariable, Meter, OutputVariable, etc. to Alfalfa so it may be consumed there.

@TShapinsky
Copy link
Member

TShapinsky commented Sep 4, 2024

@kbenne this is pretty much there.
My TODO:

  • Finish json serialization saving and connection with existing OS workflow.
  • Audit if there are additional Model Objects of IDF objects that we should support automatic point creation for
  • Add methods for exposing lists of variables from one object, as seen in EnergyManagementSystem:GlobalVariable

@kbenne
Copy link
Contributor Author

kbenne commented Sep 6, 2024

@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.

@kbenne
Copy link
Contributor Author

kbenne commented Sep 6, 2024

I created a new issue to create documentation for this new API.

@TShapinsky TShapinsky added Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. component - Ruby bindings component - Python bindings labels Sep 6, 2024
@TShapinsky
Copy link
Member

TShapinsky commented Sep 13, 2024

@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.

@kbenne
Copy link
Contributor Author

kbenne commented Sep 19, 2024

@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.

cc @DavidGoldwasser @TShapinsky

@kbenne
Copy link
Contributor Author

kbenne commented Sep 19, 2024

@wenyikuang can you investigate the CI failures?

@wenyikuang
Copy link
Collaborator

Looks the windows-incremental is lost connection from the jenkins, let me look

@TShapinsky
Copy link
Member

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")
Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

src/utilities/CMakeLists.txt Outdated Show resolved Hide resolved
@wenyikuang
Copy link
Collaborator

Resolve the issue of connection between jenkins and windows-incre-on-aws

https://ci.openstudio.net/blue/organizations/jenkins/openstudio-incremental-windows/detail/PR-5255/7/pipeline

(From another PR but using the same node)

Or are you mentioning these one?

The following tests FAILED:

	665 - BCLFixture.RemoteBCL_BCLSearchResult (Failed)

	1877 - ModelFixture.PythonPluginInstance_NotPYFile (Failed)

	2304 - ModelFixture.ScheduleFile (Failed)

	3771 - OpenStudioCLI.Check_Alfalfa (Failed)

Copy link
Collaborator

@jmarrec jmarrec left a 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).

python/engine/test/AlfalfaMeasure/measure.py Outdated Show resolved Hide resolved
python/engine/test/PythonEngine_GTest.cpp Show resolved Hide resolved
python/module/openstudio.py Outdated Show resolved Hide resolved
src/utilities/filetypes/AlfalfaJSON.cpp Outdated Show resolved Hide resolved
m_impl->exposePoint(point);
}

boost::optional<AlfalfaPoint> AlfalfaJSON::exposeFromObject(const openstudio::IdfObject& idf_object, const std::string& display_name) {
Copy link
Collaborator

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.

Copy link
Member

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.

src/utilities/filetypes/AlfalfaJSON.cpp Outdated Show resolved Hide resolved
src/utilities/filetypes/AlfalfaJSON.hpp Outdated Show resolved Hide resolved
src/workflow/RunPostProcess.cpp Outdated Show resolved Hide resolved
@jmarrec
Copy link
Collaborator

jmarrec commented Sep 20, 2024

src/utilities/filetypes/AlfalfaJSON.cpp Outdated Show resolved Hide resolved
src/utilities/filetypes/AlfalfaJSON.cpp Outdated Show resolved Hide resolved
src/utilities/alfalfa/AlfalfaPoint_Impl.hpp Outdated Show resolved Hide resolved
@jmarrec jmarrec mentioned this pull request Sep 20, 2024
19 tasks
@jmarrec
Copy link
Collaborator

jmarrec commented Sep 20, 2024

@ci-commercialbuildings
Copy link
Collaborator

ci-commercialbuildings commented Oct 11, 2024

@TShapinsky
Copy link
Member

@jmarrec I'm starting to get into the cleaning phase. I think it would be useful if you could go over the Component code (ComponentBase, AlfalfaComponent, AflalfaActuator, AlfalfaConstant, AlfalfaMeter, AlfalfaOutputVariable, AlfalfaGlobalVariable) and let me know what you think. I'm decently happy with the new architecture, but it isn't exactly something that is elsewhere in the code base so I'd like your opinion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component - Python bindings component - Ruby bindings Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants