-
Notifications
You must be signed in to change notification settings - Fork 394
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
Fix Divide by Zero in CentralHeatPump #7542
Conversation
…HP model being finished
@YanfeiNREL @lgentile it has been 28 days since this pull request was last updated. |
1 similar comment
@YanfeiNREL @lgentile it has been 28 days since this pull request was last updated. |
@Myoldmopar This has a couple of basic fixes from @YanfeiNREL. It should be ready to go. |
@YanfeiNREL @lgentile it has been 28 days since this pull request was last updated. |
I was trying to review this. I transitioned the defect file up to latest IDD and ran it with develop. I was using Melbourne weather, but I did not encounter a divide by zero. I ran with this branch and also did not encounter an issue, but I'm having trouble verifying if I can't reproduce the issue. @mjwitte I know this is pulling from a long time back but if you have any tips I'd be happy to hear them. In general, these protections are fine, so I'm OK with this going in, but I'd like it better if I could verify the fix. |
@Myoldmopar Depending on the configuration and platform divide by zero could slip by unnoticed. Have you tried running it in the debugger with a breakpoint when Qevap=0 on the affect line? If that doesn't work, I can try to reproduce here. |
I never get to any of the |
OK, I'll call it, these changes are sensible and CI is happy. I can't verify the fix though, so I'll merge this, and close the issue with a note about reopening if it is still present. |
Pull request overview
Pull Request Author
Line 2376-2380:
Line 2369 - 2373
For this issue, there are no after-fix plots, because the issue will be fixed after the updated GSHP model is being added and replacing the current model.
Reviewer
This will not be exhaustively relevant to every PR.