apiDataFullyReady has a very counter-intuitive return and does not match I/O reference docs - Githubissues

NREL / EnergyPlus

EnergyPlus™ is a whole building energy simulation program that engineers, architects, and researchers use to model both energy consumption and water use in buildings.

https://energyplus.net

Other

1.1k stars 384 forks source link

apiDataFullyReady has a very counter-intuitive return and does not match I/O reference docs #9998

Closed jmarrec closed 1 year ago

jmarrec commented 1 year ago

Issue overview

in C/C++:

0 means false
anything (eg 1) means true

apiDataFullyReady does the exact opposite

https://github.com/NREL/EnergyPlus/blob/af674b952fea904d4d0bb3af11166583e4444e37/src/EnergyPlus/api/datatransfer.cc#L125-L132

This leads to horrendous things to read like this comment: https://github.com/NREL/EnergyPlus/blob/af674b952fea904d4d0bb3af11166583e4444e37/tst/EnergyPlus/unit/api/datatransfer.unit.cc#L321

This also means that in this test, we actually NEVER get past this point and read the handles (the test does nothing):

https://github.com/NREL/EnergyPlus/blob/af674b952fea904d4d0bb3af11166583e4444e37/tst/EnergyPlus/api/TestDataTransfer.c#L64

The same code is in the I/O ref: https://github.com/NREL/EnergyPlus/blob/af674b952fea904d4d0bb3af11166583e4444e37/doc/input-output-reference/src/overview/api-usage.tex#L283

I suggest flipping the return, so that int can be converted to a bool properly

Details

Some additional details for this issue (if relevant):

Platform (Operating system, version): All
Version of EnergyPlus (if using an intermediate build, include SHA): develop, af674b952fea904d4d0bb3af11166583e4444e37
Found while fixing #9957

Checklist

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

[ ] Defect file added (list location of defect file here)
[ ] Ticket added to Pivotal for defect (development team task)
[ ] Pull request created (the pull request will have additional tasks related to reviewing changes that fix this defect)

jmarrec commented 1 year ago

Another issue I found: this returns 0, not -1 when not found: https://github.com/NREL/EnergyPlus/blob/af674b952fea904d4d0bb3af11166583e4444e37/src/EnergyPlus/api/datatransfer.cc#L820-L832

This conflicts with the public docstring (python) here: https://github.com/NREL/EnergyPlus/blob/af674b952fea904d4d0bb3af11166583e4444e37/src/EnergyPlus/api/datatransfer.py#L608-L616

and it's unlike the other getXXX methods