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

Verifications for Section 5.16.2 of ASHRAE Guideline 36 #3

Merged
merged 14 commits into from
Jul 1, 2023

Conversation

lymereJ
Copy link
Collaborator

@lymereJ lymereJ commented Jun 22, 2023

Verifications for Section 5.16.2, Supply Air Temperature, of ASHRAE Guideline 36.

Comment on lines +2 to +6
on:
push:
branches: [ develop ]
pull_request:
branches: [ '*' ]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the workflow so we only build the doc when we push on develop and/or we create a PR.

branches: [ develop ]
branches: [ '*' ]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the workflow so we run the tests on all pushes.

@lymereJ lymereJ mentioned this pull request Jun 29, 2023
Copy link
Collaborator

@leijerry888 leijerry888 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great and clean commits. Only a few comments for discussion. We can quickly go through these when we meet.

else:
return False
else:
return "Untested"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean in the comments of my PR. My only concern with this is if "Untested" will trigger a error with matplotlib trying to plot the time series. Other than that, as long as we follow the rule, either way should be okay.

return False
else:
if len(self.result[self.result == "Untested"] > 0):
return "Untested"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it only "Untested" if all samples are "Untested", or as long as one sample is "Untested"? I thought it should be the former. Maybe it depends. I modified check_bool in checklib.py in PR #4 to implement the former.

def verify(self):
self.result = self.df.apply(lambda d: self.relief_air_damper(d), axis=1)

def check_bool(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this check_bool is already implemented in RuleCheckBase. Applies to similar locations elsewhere

return False
else:
if len(self.result[self.result == "Untested"] > 0):
return "Untested"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar to an earlier comment. We may also want to test this. This is optional, just for you to consider.

@leijerry888 leijerry888 self-requested a review June 30, 2023 05:11
@@ -532,5 +532,291 @@
],
"description_verification_type": "rule-based",
"assertions_type": "pass"
}
},
"G36_SupplyAirTemperatureSetpoint": {
Copy link
Collaborator

@yunjoonjung-PNNL yunjoonjung-PNNL Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leijerry888 FYI, Jeremy and I discussed that the names should match the class names. Also, as of G36_SimultaneousHeatingCooling, there are 2 more spaces so, this will be fixed soon.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in the latest commit!

@leijerry888 leijerry888 mentioned this pull request Jun 30, 2023
@leijerry888 leijerry888 merged commit 26be55a into g36lib_dev Jul 1, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants