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

Issue1188 dampers refactor #1267

Merged
merged 41 commits into from
Feb 20, 2020
Merged

Issue1188 dampers refactor #1267

merged 41 commits into from
Feb 20, 2020

Conversation

AntoineGautier
Copy link
Contributor

@AntoineGautier AntoineGautier commented Dec 23, 2019

This PR only partially addresses #1188.

It deals with:

  • the merge of VAVBoxExponential and Exponential,
  • the suppression of dp_nominalIncludesDamper in favor of dpDamper_nominal and dpFixed_nominal.

The modification of PressureIndependent will be addressed in a next PR.

Note:

  1. For now I applied the conversion script to update the examples. That leads to low values of dpDamper_nominal ~ 0.3 Pa (former default). It is pushed as is in order to illustrate the low impact of the refactoring on the reference results. However it might be ultimately better to present to the user more representative values (10 Pa) in the examples.
  2. The suppression of k0 in favor of the leakage coefficient is not handled by the conversion script (I made the assumption that no user would have modified the default value for that parameter).
  3. I had to account for all possible partial paths in the conversion script to convert all instances e.g. Actuators.Dampers.VAVBoxExponential vavDam(...) in IBPSA.Fluid.Examples.SimpleHouse is not converted by simply using IBPSA.Actuators.Dampers.VAVBoxExponential in the conversion script.

@mwetter
Copy link
Contributor

mwetter commented Jan 10, 2020

Note the addition of getInstanceName() in the assertion message in d6f83bb.

We should do this throughout the library. Otherwise, JModelica will not write which instance triggered the assertion. I updated the style guide at
https://github.com/ibpsa/modelica-ibpsa/wiki/Style-Guide#equations-and-algorithms
(and also for the Buildings library) correspondingly.

@AntoineGautier Can you please set the new default value for dpDamper_nominal. Then it is ready to merge from my point of view after all library developers agree.

@AntoineGautier
Copy link
Contributor Author

@Mathadon @PMehrfeld @nytschgeusen This is a non backward compatible PR waiting for your approval.
Note that for Dymola users there is a conversion script available.

Copy link
Contributor

@PMehrfeld PMehrfeld left a comment

Choose a reason for hiding this comment

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

@AntoineGautier: Thank you very much for your huge effort you put into these good changes! And thanks for the good issue documentation (especially the slides were helpful).

I approve, but please address my comments and implement the suggestions. Although none of them are truly critical or impact the results. But I think it raises the usability.

Additionally, I have this comment:
All three classes on top level of the Dampers package need to parametrize the damper coefficients. But only Exponential provides this very helpful table acc. to ASHRAE 825-RP.
I suggest to move this table to
a) the two other models as well or
b) the base class model PartialDamperExponential.

If you are going with option b) you should highlight that the standard parameters are valid for opposed blades.
Additionally, you should comment that k0 is now determined by the leakage parameter l and others.

IBPSA/Fluid/Actuators/Dampers/Exponential.mo Outdated Show resolved Hide resolved
IBPSA/Fluid/Actuators/Dampers/MixingBox.mo Outdated Show resolved Hide resolved
IBPSA/Fluid/Actuators/Dampers/MixingBox.mo Outdated Show resolved Hide resolved
IBPSA/Fluid/Actuators/Dampers/MixingBox.mo Outdated Show resolved Hide resolved
@AntoineGautier
Copy link
Contributor Author

@PMehrfeld Thanks a lot for the detailed review. I addressed your comments in my last commits.

@AntoineGautier
Copy link
Contributor Author

@mwetter This is ready to merge.

@mwetter
Copy link
Contributor

mwetter commented Feb 20, 2020

@AntoineGautier : I removed the assignments k1=0.45 as this is already the default.
This is ready to be merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants