Second JOSS review issues

[FWuellhorst]

I will list the issues I encounter in this issue for the review openjournals/joss-reviews#8083 (comment).

General Review from reading the docs and paper

• Adding to the broken and missing links, mentioned in JOSS Paper Issues #87 by @lazlop, some docs mention tbd, e.g. defined on this schema (TBD).
• The brick compliance under design/ mentions classes which do not exist. Also, a dedicated example using brick would be nice to see how your code translates to actual building automation without the need for possibly error-prone user specification.
• The API doc is completely outdated and mentions some ANIMATE framework.
• "no" is not a clear name for ID or number of sequence.
• If this is not only for simulation but real cases, simulation_IO is a bad name for an API
• your code needs a lot of requirements. Are packages like black or pre-commit really used for your framework or just for your development? Consider having optional dependencies for development in that case.
• I have close to zero idea what is happening based on the output of the quick-start guide. What are y-labels?
• Running the workflow (https://pnnl.github.io/ConStrain/Quickstart%20Guide.html#using-constrain-s-workflows) gives me

Workflow done at 14:27:30, a total of 5 states were executed in 0:00:01.254253.
2025-05-26:14:27:30,678 ERROR   [verification_case.py:242] Missing required key 'parameters' in {'dev_settings': {'output_coil_heating': 'heating_output', 'output_coil_cooling': 'cooling_output', 'position_damper_air_return': 'ra_p', 'position_damper_air_return_max': 'max_ra_p', 'position_damper_air_outdoor': 'oa_p', 'position_damper_air_outdoor_min': 'min_oa_p', 'position_damper_air_outdoor_max': 'max_oa_p', 'flag_economizer_limit': 'economizer_high_limit_reached'}}
2025-05-26:14:27:30,678 ERROR   [workflow.py:524] Something is wrong in the workflow execution

Is this good or bad? The ERROR seems critical, although the print statements are ok.

• Your example gives

2025-05-26:14:38:55,423 ERROR   [verification_case.py:242] Missing required key 'parameters' in {'dev_settings': {'output_coil_heating': 'heating_output', 'output_coil_cooling': 'cooling_output', 'position_damper_air_return': 'ra_p', 'position_damper_air_return_max': 'max_ra_p', 'position_damper_air_outdoor': 'oa_p', 'position_damper_air_outdoor_min': 'min_oa_p', 'position_damper_air_outdoor_max': 'max_oa_p', 'flag_economizer_limit': 'economizer_high_limit_reached'}}
2025-05-26:14:38:55,423 ERROR   [verification.py:173] path_to_custom_tolerance_file should be a string. The default tolerances will be used.

Again: Are those critical or warnings? Especially the first sounds bad.

• Basically, you rename idf simulation outputs to the names in points and then apply some rule-based logic, or? This should be agnostic to the simulation/data engine, so I would advise to rename the code accordingly. I was first confused seeing Modelica_Jan.csv and then simulation engine idf in Simulation_IO, first expecting a Modelica output file via SPAWN. (That being said, we have a python tool ebcpy to load .mat Modelica results as DataFrame, if you should consider Modelica as Simulation source in the future).
• The GUI looks like a good start to help users actually build custom workflows. However, appearance (e.g. missing sliders for long setting see image) can be improved.

Codestyle issues / comment:

• global variables in lowercase, e.g. in example.py path
• else return are redundant
• print should be avoided if your library is intended to be used by other libraries.
• does it make sense to include all examples and demo in the python folder? A designated demo/example would be clearer from my point of view.
• Is result really a class attribute?
• Is points_list necessary as property?
• library_outdated is redundant in times of git
• Stuff like sys.path.append("..") is not required when properly using packages
• Heavy use of classes, although sometime redundant, e.g. DataAdapter
• General structure could be improved, e.g. using a dedicated ep package for ep stuff
• Various lines too long etc., black does not seem to be used even though it is a requirement for installation
• A lot of if checks can be made as one-line, e.g.

        if abs(
            data["position_damper_air_outdoor"]
            - data["position_damper_air_outdoor_max"]
        ) < self.get_tolerance("damper", "command"):
            return True
        else:
            return False

to

        return abs(
            data["position_damper_air_outdoor"]
            - data["position_damper_air_outdoor_max"]
        ) < self.get_tolerance("damper", "command")

Critical code choices

• you use eval in several places and mention an API. Combined, users could easily inject code when sending workflows, e.g. through this line left = eval(choice["Value"])

Paper comments

• General: You have a really long motivation, kind of as in a typical research paper; but you mention only briefly how ConStrain actually works and do not compare it to possibly existing tools and alternatives.
• ConStrain is made of two distinct components --> ReadMe states three components
• control related building energy code requirements -> What are those compared to control verifications? Not clear-.
• What do you mean by 4-5 Quads?
• HVAC used and then introduced again
• What are four code cycles, and why are they relevant?
• One sentence per paragraph is really short
• "to code or to a high-performance control guideline" What is the difference between code and guideline? In context of software code is ambiguous
• "ConStrain has applications to code-compliance building performance standards (BPS), and commissioning." Seems like a summary in mid-text.