-
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, Water-Side #265
ASHRAE Guideline 36, Section 4, Water-Side #265
Conversation
@gtfierro I could use a quick review for my approach in the |
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.
Couple small mistakes in the SHACL phrasing (it gets tough to keep track of the nesting of property/node shapes...), but otherwise this looks great
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.
This looks fantastic and I don't think I have any feedback on the shapes themselves.
Not sure why this decided to break now, but the tests fail because one of the templates makes reference to a fan
template but no fan
template exists. Here's the fix:
--- a/libraries/ashrae/guideline36/4.3-fan-powered.yml
+++ b/libraries/ashrae/guideline36/4.3-fan-powered.yml
@@ -6,7 +6,7 @@ fan-powered-terminal-unit:
brick:hasPart p:fan, p:damper ;
brick:hasPoint p:heating_signal, p:airflow, p:dat, p:zat .
dependencies:
- - template: fan
+ - template: sa_fan
args: {"name": "fan"}
- template: damper
args: {"name": "damper"}
4.11 Hot Water Plant Reviewline 22 Primary-only plants missed a sensor for Plant HW return temperature Pump bring line 228-236 to line 191 under the HW system gauge pressure since it is both for constant and variable primary loops. component of 4.11.2line 186 bring line 246 -255 above the vfd motor definition line 199 4.10 Chilled Water Plant NEED TO ADD REFRIGERANT TEMP SENSORS AND CONDENSER WATER DIFFERENTIAL PRESSURE SENSOR POINTS IN THE BRICK ONTHOLOGY##Line 382 since it is a condenser water on the chiller side entering and leaving functionality will change line 418 4.10.3 Primary (only) CHW Loop 4.10.6 Cooling Towers line 851 line 906 4.10.7 Condenser Water Loop 4.10.8 Waterside Economizer line1105 MISSED |
@selamHaile thanks, let me know when you're done with the review. FYI you can comment on specific line(s) in the code changes and even make suggestions for code changes. |
@MatthewSteen I am done with my review. |
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.
@selamHaile thanks for your review!
Here are my responses to your comments. Let me know if you'd like a meeting to go through these. Otherwise, please review and either reply or resolve.
I noticed that some of the line numbers in your comment above don't match the latest files so noted those as possibly outdated below. Maybe you reviewed local copies that were out-of-date, i.e. without git pull
ing.
Pump
bmotif:HVAC > bmotif:device??
The building_motif_ontology.ttl doesn't have a device, but that's something we can consider in the future.
NEED TO ADD REFRIGERANT TEMP SENSORS AND CONDENSER WATER DIFFERENTIAL PRESSURE SENSOR POINTS IN THE BRICK ONTHOLOGY##
Agree, see my notes here.
MISSED
heat exchanger pump and refer to the componenet.ttl
the plant need to check outside air temp and humidity for control
Will do.
@selamHaile I think I've addressed your review comments (see commit). Please formally review changes and either approve or make other suggestions. Thanks! |
@MatthewSteen when tracking down the issue you mentioned in #278 I found a couple small issues in the shapes. Here's the git patch you can apply to fix them
|
@gtfierro I'm writing the water-side templates now and the water-side shapes use
Do templates support this? It seems like this case falls between an optional and required dependency, i.e. putting both the above two plant types in the |
Following up on our discussion yesterday -- we don't currently have this feature, but I've made a note of it at #303 |
@gtfierro this is ready for a final review. |
As we discussed in the dev meeting today, this PR will need #306 to be addressed to fix syntax in the template bodies |
As we re-discussed, we decided to merge the G36 PRs without the sh:or template functionality. |
Separate pull request for the more complex water-side sections 4.10 and 4.11 to merge into #263.
@selamHaile I'd like your review of my interpretation of G36. Here are some additional areas of feedback. Thanks!