Fix to set the default per person outdoor air flow to zero when not used#4689
Conversation
…utdoor air methods that is supposed to use the per person field, in that case let it default to 0.00944 as before. This appears to fix the issue found with GitHub #4378. No test file included yet.
|
@JasonGlazer That just seems like it's masking the real problem that somewhere the method is not being applied correctly? |
|
@mjwitte I tried to hand trace the code and could not find a place that it was being used incorrectly but it is used many times and it is also moved into other data structures so I must have missed the problem. |
|
@JasonGlazer That's not good. This may be fine as a bandaid, but maybe not. If this does not break anything, then we will need a followup issue to make sure this object gets used correctly everywhere. @rraustad I thought you did some consolidation a while back to create a central function to compute the airflow from designspecification:outdoorair |
|
See CalcDesignSpecificationOutdoorAir and use in ZoneEquipmentManager. |
|
@rraustad Thanks but the CalcDesignSpecificationOutdoorAir function looks like it already respects the OAFlowMethod flag and the problem is somewhere in the code that does not respect that flag. I will keep looking. |
|
Where in the code? Wouldn't a CalcVentilationMechanical function solve this issue? One where unused inputs are disregarded. |
|
@JasonGlazer You first noticed this with a particular test file, correct? Was it a sizing problem or a control problem? The intent of the function @rraustad put in was to get rid of distributed duplicate (or apparently near-duplicate) code to process the OA specs. If you can find the code that went wrong for the test file and change it to use a central function, that would be a good fix for this issue. |
|
Probably right in here in around line 3448 of MixedAir: // Get mechanical ventilation |
|
@mjwitte I noticed the problem because annual energy results were changing when they should not but it also shows up in the Std62 report. |
…per person even if not using an appropriate OA flow method.
|
@mjwitte No, OAFromArea does not need a similar check because it defaults to zero when not used. I chose to minimize the amount of code changes to just a few lines rather than introduce a function that I was not familiar with that might have unexpected side effects. |
|
@JasonGlazer So, that's why you chose to do the same with the per person OA, I presume. But what happens if someone enter method = oaperperson and puts a nonzero value in the OA/area field? And these scattered places in the code - do they properly do the max option? |
…esSpecOA-PerPersonUsedWhenFlowArea
…nd added a unit test
|
@mjwitte Could you take a look at the refactoring of the GetInput routine, I was following your example. |
|
The ERR and EIO files of: DesignSpecification:OutdoorAir, This should have triggered the error message all along and not use the hidden default value for flow per person so changes to this file are probably expected. The DSOA that have this type of input are also the ones in the error message and EIO as different: |
|
@jasondegraw I made some minor whitespace changes, added the unit test to the unit test CMakeLists file, and resolved the tiny conflict from develop. I'll let CI have a pass at it and it should be good to go. |
|
Errm @JasonGlazer ... |
|
@JasonGlazer So 2 files are showing diffs. The eio diffs are probably expected; air flow rates changed, and this caused some other small numeric diffs in sizing values. But I'm guessing you didn't want these err diffs to be present in the |
|
@Myoldmopar if you look up the discussion on this issue, I think those two files were falling victim of the bug that was fixed and the results should change for them. |
Yes, I'm fine with the diffs in the eio files. But we probably don't want to muddy up the error file in our examples. I'm just thinking we should fix the idfs so that they conform to proper behavior. Right? |
|
Ok, I can change the method to flow/person instead of flow/zone and probably put an explicit value in the flow/person field. That should take care of the new ERRs shown. |
|
Thanks, I was hoping it was a simple fix like that to clean up the err file. |
|
@Myoldmopar These errors would hope with the code coverage stats! But I agree, we should probably clean these up. Question is, what was the intent? |
Can you clarify what you are looking for? What was the intent of the example file inputs? What was the intent of the fix? And yes, they would add to the coverage stats 👍, but not worth it... 👎 |
|
What was the intent of the example files? |
|
Errm, to demonstrate |
…o DSOA. The old versions of the example files showed as flow per zone but because the flow per person was field was defaulting to a value and being used in the calculations it was actually providing flow. The change to the example files explicitly is providing the same amount of flow and is shown as flow per person.
|
@Myoldmopar I just updated the example files that were creating diffs in the err files. Does the CI get triggered to run when example files change or only when source changed? |
|
Triggered on every commit; you should be good to go. |
|
@wfbuhl @mjwitte @JasonGlazer As far as I can tell from looking over the CI results, the only issue remaining is the ExpandObjects update. Is anyone available to make those changes, or do I need to venture over there? |
|
@wfbuhl @mjwitte @JasonGlazer SORRY, wrong pull request page open...ignore the previous. |
…enFlowArea Fix for 4378 that sets the default per person to zero when not used.
Myoldmopar
changed the title
5 participants
Set the default for per person flow to zero unless it is one of the outdoor air methods that is supposed to use the per person field, in that case let it default to 0.00944 as before. This appears to fix the issue found with GitHub #4378. No test file included yet.