[Developer] Adjust alfalfa_utility #5259

jmarrec · 2024-09-20T14:20:11Z

Pull request overview

Adjustments for Add Utility for Generating Alfalfa Metadata to OpenStudio #5236

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

…alfa measure

jmarrec · 2024-09-20T14:20:46Z

src/utilities/alfalfa/AlfalfaActuator.hpp

@@ -2,8 +2,7 @@
 #define ALFALFA_COMPONENT_ACTUATOR_HPP

 #include "AlfalfaComponent.hpp"
-#include "utilities/idf/IdfObject.hpp"
-#include "utilities/core/Logger.hpp"


Logger was included everywhere but not used. Because you throw std::runtime_error.

Maybe you should use a proper Logger and use LOG_AND_THROW, one of the two.

I'm not sure what you mean by "use a proper Logger and use LOG_AND_THROW, one of the two" do you mean or? Can you provide an example of somewhere in the OS code where the pattern you'd like me to use is?

jmarrec · 2024-09-20T14:20:56Z

src/utilities/filetypes/AlfalfaJSON.cpp


-    AlfalfaJSON_Impl::AlfalfaJSON_Impl(const std::string& s) {}
+    AlfalfaJSON_Impl::AlfalfaJSON_Impl(const std::string& s) {
+      // TODO: use s


jmarrec · 2024-09-20T14:21:00Z

src/utilities/filetypes/AlfalfaJSON.cpp


-    AlfalfaJSON_Impl::AlfalfaJSON_Impl(const openstudio::path& p) {}
+    AlfalfaJSON_Impl::AlfalfaJSON_Impl(const openstudio::path& p) {
+      // TODO: use p


jmarrec · 2024-09-20T14:21:06Z

src/utilities/alfalfa/AlfalfaPoint_Impl.hpp

@@ -17,10 +17,13 @@ namespace alfalfa {
      AlfalfaPoint_Impl();

     protected:
+      // TODO: is this pImpl is needed? If so, add getters and setters and make all members private.


Sounds good, I was talking to Kyle about this. I feel like it is a bit more messy to have every getter and setter written twice, but since that seems to be the pattern here there's probably something I'm missing.

It's annoying to write for sure. For ModelObjects we actually have ruby scripts that will do 90% of the work for us and generate the pimpl class: https://github.com/NREL/OpenStudio/blob/develop/developer/ruby/GenerateClass.rb and https://github.com/NREL/OpenStudio/blob/develop/developer/doc/Adding_Model_Object_to_OpenStudio.docx

I had written this when I first touched the OpenStudio codebase: https://github.com/jmarrec/OpenStudio-Tutorial-for-Cpp-Noobs?tab=readme-ov-file#pimpl

a few years later, I think the pImpl pattern in the case of model objects at least is useful for us because:

The Impl methods are not exposed to SWIG (ruby, python, C# bindings)

Technically you can change the Impl header without changing the public header, which is good because:

If a class never included the Impl header (and we have to do that too often for casting especially), then there's a net compilation time benefit

It forces us to treat the public header as a sacred API, and non-backward compatible changes must be really motivated. We often remove methods from the Impl when they become moot, and we'll put a [[deprecated]] method in the public side for 2 or 3 releases with a clear message that it'll be removed.

That makes sense. The reason I used the pImpl pattern here was to allow the point object a user is given after creation and the point object in the points vector within AlflalfaJSON to all point to the same underlying data. This seemed like the best option outside of dealing with just raw or lightly wrapped pointers. If there is a better paradigm you have in mind for this I would be happy to implement it.

jmarrec · 2024-09-20T14:21:14Z

src/utilities/filetypes/AlfalfaJSON.hpp

@@ -100,7 +99,7 @@ namespace alfalfa {
    std::vector<AlfalfaPoint> getPoints();

   private:
-    boost::optional<std::string> getName(const openstudio::IdfObject& idf_object);
+    static boost::optional<std::string> getName(const openstudio::IdfObject& idf_object);


static a few methods

TShapinsky

Thanks for this @jmarrec just a couple questions if you have time.

TShapinsky · 2024-09-20T21:45:41Z

python/engine/test/PythonEngine_GTest.cpp

+    }
+  ]
+}
+  )json";


Nice, what is the json(...)json a directive to?

Nothing, it's an optional arbitrary string, at most 16 characters long. I like to use it, when this is a json thing I put json, when it's a sql query I put sql... etc.

(technically speaking you could configure your editor to pick that up and apply a different syntax highlighting based on it)

https://en.cppreference.com/w/cpp/language/string_literal

R"d-char-seq (optional)(r-char-seq (optional))d-char-seq (optional)"

d-char-seq - A sequence of one or more d-char s, at most 16 characters long
d-char - A character from the basic character set, except parentheses, backslash and spaces

TShapinsky · 2024-09-20T21:46:40Z

src/utilities/alfalfa/AlfalfaActuator.cpp

-#include "utilities/idd/IddObject.hpp"
-#include "utilities/idd/IddEnums.hpp"
+#include "../idd/IddObject.hpp"
+#include "../idd/IddEnums.hpp"


So the convention is to use relative paths for project imports? I was opting for absolute as it doesn't break everything when you move files around.

That's the way all other files are as far as I can tell. It's debatable whether it's a good idea, but I'd prefer consistency.

(Bit of context: We don't move things often (if at all), and I am not afraid of spending 15 minutes to write a python script with a regex or two to fixup the paths if we need to move things)

that sounds good, currently there a question on the other PR from kyle as to whether alfalfa should be moved out of utilities to be on the same level. Ideally I think I'd wait for that before switching everything back to relative, but as you noted it's not that big of a lift to fix.

TShapinsky · 2024-09-20T21:47:17Z

src/utilities/alfalfa/AlfalfaActuator.cpp

-      component_name = actuator.getString(OS_EnergyManagementSystem_ActuatorFields::ActuatedComponentName);
-      component_type = actuator.getString(OS_EnergyManagementSystem_ActuatorFields::ActuatedComponentType);
-      control_type = actuator.getString(OS_EnergyManagementSystem_ActuatorFields::ActuatedComponentControlType);
+      component_name = actuator.getString(OS_EnergyManagementSystem_ActuatorFields::ActuatedComponentName).value_or("");


didn't know about this pattern, seems like a good improvement.

https://live.boost.org/doc/libs/1_79_0/libs/optional/doc/html/boost_optional/reference/header__boost_optional_optional_hpp_/detailed_semantics___optional_values.html

TShapinsky · 2024-09-20T21:48:46Z

src/utilities/alfalfa/AlfalfaActuator.hpp

@@ -2,8 +2,7 @@
 #define ALFALFA_COMPONENT_ACTUATOR_HPP

 #include "AlfalfaComponent.hpp"
-#include "utilities/idf/IdfObject.hpp"
-#include "utilities/core/Logger.hpp"


I'm not sure what you mean by "use a proper Logger and use LOG_AND_THROW, one of the two" do you mean or? Can you provide an example of somewhere in the OS code where the pattern you'd like me to use is?

TShapinsky · 2024-09-20T21:49:19Z

src/utilities/alfalfa/AlfalfaComponent.cpp


 namespace openstudio {
 namespace alfalfa {

-  AlfalfaComponent::AlfalfaComponent(const std::string& type, const Capability capabilities) : type(type), capabilities(capabilities) {}
+  AlfalfaComponent::AlfalfaComponent(const std::string& type, Capability capabilities) : type(type), capabilities(capabilities) {}


this is fair, since it's just a uint under the hood

yes, it's a POD type [1] that'll fit in a register. I tried adding a clang-tidy check for that in LLVM (and someone else did independently) but it never got in.

[1] This may be an unsigned int, and it is on GCC for eg (cf https://gcc.godbolt.org/z/47z5z3bP8), but it's actually implementation defined per the cpp standards.

TShapinsky · 2024-09-20T21:51:06Z

src/utilities/alfalfa/AlfalfaPoint.cpp

@@ -97,36 +103,36 @@ namespace alfalfa {
  Json::Value AlfalfaPoint::toJSON() const {
    Json::Value point;

-    if (id().is_initialized()) {
-      point["id"] = id().get();
+    if (auto id_ = id()) {


oh cool, I didn't realize you can do assignment in the if

boost::optional has an operator bool(), which is going to be used here.

FYI, C++17 added init-statement to if. So you could do something like

if (auto x = func(); x.condition()) {

https://en.cppreference.com/w/cpp/language/if

TShapinsky · 2024-09-20T21:54:44Z

src/utilities/alfalfa/AlfalfaPoint.cpp

 namespace openstudio {
 namespace alfalfa {
+
+  static constexpr std::string_view ID_VALID_CHARS_MSG = "IDs can only contain letters, numbers, and the following special characters _-[]():";


That makes sense to move this out of the class.

Let me make sure I'm understanding this correctly. The purpose of using a constexpr std::string_view is that this gets populated in memory and returns a pointer at compile time so that each time the string is accessed it doesn't need to build a new one?

this is a global static constexpr variable. I could omit the static, and it'd still be static because it's declared at global scope and it's constexpr, but this is more explicit.

And so it initializes it in static storage (the .rodata section of the binary). The string_view is materialized in the binary.

see this short example: https://gcc.godbolt.org/z/o7veTMETP

and if you want to learn more about constexpr, I suggest C++ weekly, the start of this episode maybe: https://www.youtube.com/watch?v=IDQ0ng8RIqs

TShapinsky · 2024-09-20T21:57:56Z

src/utilities/alfalfa/AlfalfaPoint_Impl.hpp

@@ -17,10 +17,13 @@ namespace alfalfa {
      AlfalfaPoint_Impl();

     protected:
+      // TODO: is this pImpl is needed? If so, add getters and setters and make all members private.


Sounds good, I was talking to Kyle about this. I feel like it is a bit more messy to have every getter and setter written twice, but since that seems to be the pattern here there's probably something I'm missing.

TShapinsky · 2024-09-20T22:00:50Z

src/utilities/filetypes/AlfalfaJSON.cpp


  boost::optional<AlfalfaPoint> AlfalfaJSON::exposeConstant(float value, const std::string& display_name) {
-    AlfalfaConstant component(value);
-    return exposeFromComponent(component, display_name);
+    return exposeFromComponent(AlfalfaConstant{value}, display_name);


I'm not familiar with this method of constructing, this is just a way to construct the class inline in the stack?

This is by definition a temporary (because it does not have a name), so it's an R-value (a prvalue I think), and I know that there will be no copies.

In this specific case, this is unnecessary, because the exposeFromComponent actually takes a const-ref, so no copy or move will happen, but that may change... It didn't hinder readability much here, so I just went with it.

This is heavily related to the pass-by-value and move idiom.

Here is something to play for you, switch the defines on line 41 and 42: https://gcc.godbolt.org/z/911xK1bT6

You'll see that if exposeFromComponent takes by-value:

If you NAME your variable, the Ctor is called, then the Copy Ctor

If you use a temporary (like I did here), the variable is actually constructed directly in the exposeFromComponent scope and you save a copy.

jmarrec · 2024-09-23T08:46:51Z

@TShapinsky I took some time to give (probably too) detailed explanations in the hope that it's useful context and references.

Anyways, you're responsible for the PR / branch I'm pointing this to, so it's up to you to merge this one or not (I don't know if you have merge rights though, but I can click the button if you ask me to)

jmarrec added 4 commits September 20, 2024 15:12

Raw string litteral and absolute minimal testing for PythonEngine alf…

7b00836

…alfa measure

Fixup AlfalfaJSON code

a8975c3

Lint AlfalfaPoint

2d380ce

Lint up the rest

fc08324

jmarrec added the Developer Issue label Sep 20, 2024

jmarrec requested a review from TShapinsky September 20, 2024 14:20

jmarrec self-assigned this Sep 20, 2024

jmarrec commented Sep 20, 2024

View reviewed changes

jmarrec mentioned this pull request Sep 20, 2024

Add Utility for Generating Alfalfa Metadata to OpenStudio #5236

Open

19 tasks

TShapinsky approved these changes Sep 20, 2024

View reviewed changes

TShapinsky merged commit 2e67d5c into alfalfa_utility Sep 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Developer] Adjust alfalfa_utility #5259

[Developer] Adjust alfalfa_utility #5259

jmarrec commented Sep 20, 2024

jmarrec Sep 20, 2024

TShapinsky Sep 20, 2024

jmarrec Sep 20, 2024

jmarrec Sep 20, 2024

jmarrec Sep 20, 2024

TShapinsky Sep 20, 2024

jmarrec Sep 23, 2024

TShapinsky Sep 23, 2024

jmarrec Sep 20, 2024

TShapinsky left a comment

TShapinsky Sep 20, 2024

jmarrec Sep 23, 2024

TShapinsky Sep 20, 2024

jmarrec Sep 23, 2024

TShapinsky Sep 23, 2024

TShapinsky Sep 20, 2024

jmarrec Sep 23, 2024

TShapinsky Sep 20, 2024

TShapinsky Sep 20, 2024

jmarrec Sep 23, 2024

TShapinsky Sep 20, 2024

jmarrec Sep 23, 2024

TShapinsky Sep 20, 2024

jmarrec Sep 23, 2024

TShapinsky Sep 20, 2024

TShapinsky Sep 20, 2024

jmarrec Sep 23, 2024 •

edited

Loading

jmarrec commented Sep 23, 2024

[Developer] Adjust alfalfa_utility #5259

[Developer] Adjust alfalfa_utility #5259

Conversation

jmarrec commented Sep 20, 2024

Pull request overview

Pull Request Author

Review Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TShapinsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarrec Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

jmarrec commented Sep 23, 2024

jmarrec Sep 23, 2024 •

edited

Loading