Skip to content

Initialize HPWHsim presets from embedded CBOR #551

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

Open
wants to merge 42 commits into
base: main
Choose a base branch
from

Conversation

spahrenk
Copy link
Contributor

@spahrenk spahrenk commented Jun 4, 2025

The initialization of HPWH models is changed to load from a hpwh_data_model representation embedded within the HPWHsim source code in CBOR format, rather than via separate code-based setup for each model. These representations were themselves generated from the original code-based setups, which were converted to JSON, then embedded as CBOR. These JSON model files are included with the HPWHsim repository at test/model_json. The embedded representations are generated from these JSON files during the CMake build. Generation is done from python using the uv package manager, which is installed during build.

@spahrenk spahrenk self-assigned this Jun 4, 2025
@spahrenk spahrenk marked this pull request as draft June 4, 2025 19:41
@spahrenk spahrenk requested a review from nealkruis June 9, 2025 22:11
@spahrenk spahrenk marked this pull request as ready for review June 13, 2025 22:51
Copy link
Contributor

@nealkruis nealkruis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few issues here that still need to be resolve. Also needs to be updated with latest main.

Comment on lines 56 to 59
- name: Install uv
uses: astral-sh/setup-uv@v5
- name: Set up Python
run: uv python install
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the same setup as HPWHsim.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only update report files with meaningful differences (i.e., not lines preceded with a !).


Yr 29526 0 0 0 28885 43.129 597.81 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
Yr 29547 0 0 0 28908 40.590 597.81 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an expected change? It's more than a a few decimal places.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file also has bigger than expected differences that need to be justified.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nealkruis The differences are primarily due to the use of grid representations replacing polynomials in performance maps, and the limited ability to reproduce a quadratic function with a cubic spline. I have made extensive effort to minimize these difference.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file, too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's happening in this file?

Phil Ahrenkiel and others added 11 commits June 20, 2025 16:31
…r-hpwh

# Conflicts:
#	test/ref-macos64-appleclang/CHDHW.REP
#	test/ref-macos64-appleclang/PERFMAP.REP
#	test/ref-macos64-appleclang/SUBMETER.REP
#	test/ref-macos64-appleclang/WSHP.REP
#	test/ref-win32-msvc/CHDHW.REP
@spahrenk spahrenk requested a review from nealkruis July 1, 2025 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants