Integrate breaking changes in btwxt interface, including error logging via interface injection. #10160

tanaya-mankad · 2023-08-21T20:41:32Z

Pull request overview

Fixes Btwxt error handler is a global object #10163 and Automatic normalization does not work when multiple Table:Lookups are added to the same Table:IndependentVariableList #10164

The btwxt interpolation library underwent a major restructuring, and changes needed to be propagated to client code. Changes include new constructors, removal of internal intermediate objects from the public interface, and most importantly, a per-instance error handling policy that uses a custom error-handling class injected into the btwxt constructors (rather than the previous global error handling callback mechanism.) The libtk205 dependency was also updated, as it depends on btwxt.

Pull Request Author

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process

Reviewer

This will not be exhaustively relevant to every PR.

Perform a Code Review on GitHub
If branch is behind develop, merge develop and build locally to check for side effects of the merge
If feature, test running new feature, try creative ways to break it
CI status: all green or justified
Check that performance is not impacted (CI Linux results include performance check)
Run Unit Test(s) locally
Check any new function arguments for performance impacts

…-btwxt-error-handling

…d entries.

Myoldmopar

I had a few comments, but nothing major. The most major is probably the question about the severe error and whether it leads to a fatal. But this PR is still marked as draft, so I'll just leave this for now.

Myoldmopar · 2023-08-29T14:41:09Z

CMakeLists.txt

@@ -253,7 +253,7 @@ add_subdirectory(third_party)
 target_include_directories(project_options INTERFACE ${PROJECT_SOURCE_DIR}/src)
 target_include_directories(project_options INTERFACE ${PROJECT_SOURCE_DIR}/third_party)
 target_include_directories(project_options INTERFACE ${PROJECT_SOURCE_DIR}/third_party/fmt-8.0.1/include)
-target_include_directories(project_options INTERFACE ${PROJECT_SOURCE_DIR}/third_party/btwxt/src)
+target_include_directories(project_options INTERFACE ${PROJECT_SOURCE_DIR}/third_party/btwxt/include)


Myoldmopar · 2023-08-29T14:42:53Z

src/EnergyPlus/AirflowNetwork/src/Solver.cpp

@@ -6941,9 +6941,9 @@ namespace AirflowNetwork {

        std::vector<Real64> dirs30 = {0, 30, 60, 90, 120, 150, 180, 210, 240, 270, 300, 330, 360};
        std::vector<Btwxt::GridAxis> dirs30Axes;
-        dirs30Axes.emplace_back(dirs30, Btwxt::Method::LINEAR, Btwxt::Method::LINEAR, std::pair<double, double>{0.0, 360.0});
+        dirs30Axes.emplace_back(dirs30, "", Btwxt::Method::linear, Btwxt::Method::linear, std::pair<double, double>{0.0, 360.0});


This is fine, but I do wonder what the new empty string constructor argument will be as I read along...

These have now been populated. I'm not sure if the string literal should be pulled out into its own variable; if it moved beyond the two local uses I'd do that.

Myoldmopar · 2023-08-29T14:43:43Z

src/EnergyPlus/CMakeLists.txt

@@ -757,6 +757,7 @@ target_link_libraries(
         DElight
         libkiva
         btwxt
+         courierr


What is courierr?

The target link didn't need to be here, so it's been removed. But, courierr is Big Ladder's current lightweight error-logging interface. The idea is that a client user would provide a concrete implementation of the courierr interface as an injected dependency, rather than an old C-style callback, to handle logging.
At the moment, there is a bit of a proliferation of those concrete implementations (in this and an upcoming PR). Neal and I intend to merge those into a single central class that anything in E+ can use.

Myoldmopar · 2023-08-29T14:45:41Z

src/EnergyPlus/CurveManager.cc

+                            thisCurve->TableIndex = gridIndex;
+                            thisCurve->GridValueIndex = state.dataCurveManager->btwxtManager.addOutputValues(gridIndex, lookupValues);
+                        } catch (Btwxt::BtwxtException &e) {
+                            ShowSevereError(state, "GridAxis construction error.");


Should this lead to a fatal error? Does it?

Yes, and fixed.

Myoldmopar · 2023-08-29T14:47:32Z

third_party/btwxt/docs/figs/curve.svg

IMHO, I wish that the third party libs had "lightweight" releases that could be brought into our repo without docs/tests/readme/etc. Just the code, dependencies, and build rules. It's not a big deal, just a comment.

The last check-in should be more lightweight.

Myoldmopar · 2023-08-29T14:49:11Z

third_party/libtk205/vendor/json/CMakeLists.txt

I actually thought that nlohmann provided a single-file include-only header option. I could definitely be wrong there though. But if that was the case, it would be really nice to just use that.

Modified as discussed on Slack; we'll see if everything is still happy.

…source.

…-btwxt-error-handling

… get initialized with RSes.

…-btwxt-error-handling

tanaya-mankad · 2023-10-02T16:55:01Z

Currently, the Windows 10 build shows a build warning emanating from alphanum.hpp. The cause appears to be the custom E+ use of alphanum in the nlohmann json header; the EnergyPlus implementation of json changed nlohmann's std::less to doj::alphanum_less. Our own third-party libraries rely on the original source implementation. I'm not really clear what the best solution here is.

Myoldmopar · 2023-10-17T18:58:07Z

There is something wrong with the error reporting here...check out this ERR diff http://energyplus.s3-website-us-east-1.amazonaws.com/regressions/2023-10/7636e6b3e972dad33c41815304938b2139dde367-1bf2590b9b5581199ecd76c704e2b5b445ef2846/ASHRAE901_OfficeLarge_STD2019_Denver_Chiller205/x86_64-MacOS-10.17-clang-14.0.0/eplusout.err.diff.html

Myoldmopar · 2023-10-17T19:09:29Z

@mbadams5 do you have a second to look at the compiler warning being emitted from doj/alphanum on Windows? This PR doesn't change that file that I can see, but something is causing it to be emitted. It is an odd looking expression in there, but that code looks like magic in a number of places, so that's not surprising.

tanaya-mankad · 2023-10-17T19:42:59Z

There is something wrong with the error reporting here...check out this ERR diff http://energyplus.s3-website-us-east-1.amazonaws.com/regressions/2023-10/7636e6b3e972dad33c41815304938b2139dde367-1bf2590b9b5581199ecd76c704e2b5b445ef2846/ASHRAE901_OfficeLarge_STD2019_Denver_Chiller205/x86_64-MacOS-10.17-clang-14.0.0/eplusout.err.diff.html

Wow, there sure is. I'm afraid I don't remember if this was a known issue that I addressed or not. I'll look into it.

tanaya-mankad · 2023-10-18T16:42:07Z

Looks like I'd missed an implementation detail! I'm checking in a fix now.

Myoldmopar · 2023-11-14T15:29:04Z

@tanaya-mankad thanks for fixing this up. It looks totally ready. I've got several PRs that I need to line up for merging, but it appears that I don't need anything else here. This should merge today.

tanaya-mankad · 2023-11-14T15:30:01Z

Thank you @Myoldmopar !

Myoldmopar · 2023-11-14T20:22:58Z

This has waited long enough, and tests out all happily here, merging. Thanks @tanaya-mankad

tanaya-mankad added 15 commits April 4, 2023 13:19

Initial pass through phase 1 btwxt changes.

47d3965

Secondary pass through btwxt changes.

35f39dd

Merge from NREL develop.

0dc6786

Add updated libtk205.

7288b78

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into fix…

4c03753

…-btwxt-error-handling

Merge from NREL develop.

d77dba9

Commit btwxt updates.

3981187

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into fix…

cb764ba

…-btwxt-error-handling

Fix merge.

6e430b1

Clean up dependency build processes.

9e9311e

Address libtk205 build warnings.

935ee45

Enable inclusion of json subdirectory by ignoring all of its contents.

aacc913

Update libtk205 and submodules.

68d5324

Update btwxt files.

858fff0

Merge from NREL/develop.

7f508cc

nealkruis added the NotIDDChange Code does not impact IDD (can be merged after IO freeze) label Aug 21, 2023

tanaya-mankad added 2 commits August 21, 2023 15:25

Fix clang formatting.

3d5413b

Use correct branch of libtk205 this time.

f667f90

nealkruis added the Defect Includes code to repair a defect in EnergyPlus label Aug 22, 2023

Btwxt interface change reversed interpolation and extrapolation metho…

89f2058

…d entries.

Myoldmopar reviewed Aug 29, 2023

View reviewed changes

Myoldmopar added this to the EnergyPlus 24.1 IOFreeze milestone Sep 11, 2023

tanaya-mankad added 7 commits September 11, 2023 10:49

Import latest btwxt and libtk205; propagate breaking changes into E+ …

14d409f

…source.

Fix formatting.

71c30a3

Test JSON_USE_IMPLICIT_CONVERSIONS=OFF.

fc3fcd9

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into fix…

399fef0

…-btwxt-error-handling

Return to json implicit conversions.

a7dba61

Give Ashrae_205 compile unit its own class; make Courierr a member to…

5f74ec3

… get initialized with RSes.

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into fix…

bf6687a

…-btwxt-error-handling

tanaya-mankad marked this pull request as ready for review September 15, 2023 14:02

tanaya-mankad added 2 commits September 22, 2023 09:15

Merge from develop and fix conflict.

8f9468c

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into fix…

062ca5d

…-btwxt-error-handling

nealkruis modified the milestones: EnergyPlus 24.1 IOFreeze, EnergyPlus 23.2 Sep 22, 2023

tanaya-mankad added 4 commits September 27, 2023 11:19

Consolidate EnergyPlusLogger impementations into one.

296a9da

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into fix…

591a9e9

…-btwxt-error-handling

Keep persistent context for penumbra error logger.

3c8d089

Comment note on #endif.

1bf2590

Add minimum logging level to EnergyPlusLogger.

498d40a

tanaya-mankad added 3 commits October 18, 2023 11:17

Clang-tidy and style fixes.

62b88dc

Fix enum according to rules.

404bda8

Fix clang format in logger implementation.

448c283

Myoldmopar self-assigned this Oct 25, 2023

tanaya-mankad added 2 commits November 1, 2023 13:55

Suppress C4706 warning on Windows.

6b62b84

Merge from NREL/develop.

75b2e49

nealkruis assigned tanaya-mankad Nov 8, 2023

nealkruis modified the milestones: EnergyPlus 23.2, EnergyPlus 24.1 Nov 8, 2023

Myoldmopar merged commit 08e876b into NREL:develop Nov 14, 2023
4 checks passed

Myoldmopar deleted the fix-btwxt-error-handling branch November 14, 2023 20:23

nealkruis mentioned this pull request Nov 27, 2023

Automatic normalization does not work when multiple Table:Lookups are added to the same Table:IndependentVariableList #10164

Closed

3 tasks

jmarrec mentioned this pull request Dec 22, 2023

Remove useless stuff in third_party folders #10343

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate breaking changes in btwxt interface, including error logging via interface injection. #10160

Integrate breaking changes in btwxt interface, including error logging via interface injection. #10160

tanaya-mankad commented Aug 21, 2023 •

edited by nealkruis

Loading

Myoldmopar left a comment

Myoldmopar Aug 29, 2023

Myoldmopar Aug 29, 2023

tanaya-mankad Sep 11, 2023

Myoldmopar Aug 29, 2023

tanaya-mankad Sep 11, 2023 •

edited

Loading

Myoldmopar Aug 29, 2023

tanaya-mankad Sep 11, 2023

Myoldmopar Aug 29, 2023

tanaya-mankad Sep 11, 2023

Myoldmopar Aug 29, 2023

tanaya-mankad Sep 11, 2023

tanaya-mankad commented Oct 2, 2023

Myoldmopar commented Oct 17, 2023

Myoldmopar commented Oct 17, 2023

tanaya-mankad commented Oct 17, 2023

tanaya-mankad commented Oct 18, 2023 •

edited

Loading

Myoldmopar commented Nov 14, 2023

tanaya-mankad commented Nov 14, 2023

Myoldmopar commented Nov 14, 2023

Integrate breaking changes in btwxt interface, including error logging via interface injection. #10160

Integrate breaking changes in btwxt interface, including error logging via interface injection. #10160

Conversation

tanaya-mankad commented Aug 21, 2023 • edited by nealkruis Loading

Pull request overview

Pull Request Author

Reviewer

Myoldmopar 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

tanaya-mankad Sep 11, 2023 • edited Loading

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

tanaya-mankad commented Oct 2, 2023

Myoldmopar commented Oct 17, 2023

Myoldmopar commented Oct 17, 2023

tanaya-mankad commented Oct 17, 2023

tanaya-mankad commented Oct 18, 2023 • edited Loading

Myoldmopar commented Nov 14, 2023

tanaya-mankad commented Nov 14, 2023

Myoldmopar commented Nov 14, 2023

tanaya-mankad commented Aug 21, 2023 •

edited by nealkruis

Loading

tanaya-mankad Sep 11, 2023 •

edited

Loading

tanaya-mankad commented Oct 18, 2023 •

edited

Loading