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

NewFeature: Add measurement unit output for API #10367

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

dogganon
Copy link
Contributor

Pull request overview

This PR adds units of measurement to the output of getAPIData (python: get_api_data) and listAllAPIDataCSV (python: list_available_api_data_csv) in the data transfer API. A new field named unit is added to the class APIDataEntry (python: APIDataExchangePoint) and a new column is added to the CSV for variables with units.

The purpose of this PR is to provide additional (and critical!) information to the API users.
The absence of such information can have serious consequences (see https://usma.org/unit-mixups#locale-notification), including the loss of lives.

Users should NOT rely on external documents for this type of information. The API should be the authoritative single source of truth.

@Myoldmopar Myoldmopar self-assigned this Jan 31, 2024
@Myoldmopar Myoldmopar added IDDChange Code changes impact the IDD file (cannot be merged after IO freeze) APIChange Code changes impact the Python Plugins, C API or Python API Bindings labels Feb 14, 2024
@Myoldmopar
Copy link
Member

So actuators and internal variables are coming out of the CSV with the units in square brackets. Meters and output variables are coming out without them:

image

image

Feels like it is worth making them consistent. I don't really care which way we go, the brackets feel a smidge unnecessary, but I don't care either way as long as they are consistent.

@rraustad
Copy link
Contributor

The rdd reports with square brackets:

Zone,Average,Site Outdoor Air Drybulb Temperature [C]
Zone,Average,Site Outdoor Air Dewpoint Temperature [C]
Zone,Average,Site Outdoor Air Wetbulb Temperature [C]
Zone,Average,Site Outdoor Air Humidity Ratio [kgWater/kgDryAir]

as well as many warning messages:

** Warning ** In zone GUID20_ZN_ROOM_00_02 there is unbalanced air flow. Load due to induced outdoor air is neglected.
**   ~~~   **  Environment=PRIMARY, at Simulation time=01/01 00:00 - 01:00
**   ~~~   **   Flows [m3/s]: Inlets: 0.316122  Unbalanced exhausts: 0.471947  Returns: 0.000000

@Myoldmopar
Copy link
Member

That's fine, like I said, I don't have any problem with the square brackets, I just want this new output to be consistent throughout all the different categories 👍

@dogganon
Copy link
Contributor Author

@Myoldmopar

I just want this new output to be consistent throughout all the different categories

Apparently the unit definitions are all over the place, which explains the inconsistency. Ideally we should have one single enum + string helper class shared by all subsystems and exposed by the API. The current API obviously would need to be revamped to accommodate enums though.. (swig could be used to reduce much of the boilerplate in the API code)

@Myoldmopar Myoldmopar added NewFeature Includes code to add a new feature to EnergyPlus and removed IDDChange Code changes impact the IDD file (cannot be merged after IO freeze) labels Feb 16, 2024
@Myoldmopar
Copy link
Member

@chen1682-ntu-edu-sg could you confirm whether you've submitted a contribution policy agreement for EnergyPlus? In order for us to accept these changes, we'll need to get that in place, if it's not already. I couldn't find it in my list, but perhaps I just missed it.

You can find information here, and you can also email me: edwin dot lee at nrel dot gov, and I can help you out.

I am willing to push some fixes to get this closer to merge, but don't want to go down that route until the agreement is in place.

@dogganon
Copy link
Contributor Author

dogganon commented Feb 20, 2024

@Myoldmopar

could you confirm whether you've submitted a contribution policy agreement for EnergyPlus?

Yep. See #10219 (comment).
On a side note - you should probably use https://github.com/cla-assistant/cla-assistant to automatically keep track of those agreements. Emails are just too unreliable.

@Myoldmopar Myoldmopar added this to the EnergyPlus 24.2 milestone Mar 14, 2024
@nrel-bot
Copy link

@Myoldmopar @chen1682-ntu-edu-sg @Myoldmopar it has been 28 days since this pull request was last updated.

1 similar comment
@nrel-bot-2b
Copy link

@Myoldmopar @chen1682-ntu-edu-sg @Myoldmopar it has been 28 days since this pull request was last updated.

@nrel-bot-2c
Copy link

@Myoldmopar @dogganon @Myoldmopar it has been 28 days since this pull request was last updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APIChange Code changes impact the Python Plugins, C API or Python API Bindings NewFeature Includes code to add a new feature to EnergyPlus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants