-
Notifications
You must be signed in to change notification settings - Fork 7
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
ASHRAE Guideline 36, Section 4 #263
base: develop
Are you sure you want to change the base?
Conversation
@gtfierro could you take a quick look at the FCU shapes and provide some feedback before I move on to the more complex water-side systems? |
library: https://brickschema.org/schema/1.3/Brick | ||
args: {"name": "zone"} | ||
- template: fan | ||
args: {"name": "fcu_fan"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The values of the args
dictionary need to appear as parameters in the body. So, above, you'd want to change line 7 to have p:fcu_fan
, or change the args here to read as args: {"name": "fcu"}
(I definitely need to go and create an LSP for these template files to avoid these kinds of issues...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same deal with the clg_coil
, htg_coil
and filter
params/dependencies here too
dependencies: | ||
- template: https://brickschema.org/schema/Brick#Filter_Differential_Pressure_Sensor | ||
library: https://brickschema.org/schema/1.3/Brick | ||
args: {"name": "filter_pd"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filter_pd
should appear somewhere in the body, or you should change filter_pd
here to pressure_drop
The shapes look great! The templates have a couple issues which should be easily resolvable -- let me know if my comments aren't clear |
I'm going to add the CHW (4.10) and HW (4.11) Plants on a separate branch and open a pull request to merge into this one. |
@gtfierro not at all. I noticed the |
…into guideline36-section4
Most of the tests are passing now. There's one more unit test which is failing due to some changes in Brick 1.4. I should be able to fix this tomorrow. |
This reverts commit 1a42b32.
…into guideline36-section4
The integration tests take >3h so I think we should refactor our CI/CD after the release for quick feedback (e.g. only testing with Python 3.11). |
The docs build from this branch have some issues so I'm going to evaluate the develop docs to determine if it'd be less work to get those working for the release rather than merging this, i.e. wait to merge this until after the release. |
Close #250. Adds the remaining shapes and templates and cleans up the existing ones.
Add
Clean