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

Circular parameter assignment in Buildings.Fluid.HeatPumps.Carnot_TCon #1692

Closed
mwetter opened this issue Feb 3, 2023 · 5 comments · Fixed by #1693
Closed

Circular parameter assignment in Buildings.Fluid.HeatPumps.Carnot_TCon #1692

mwetter opened this issue Feb 3, 2023 · 5 comments · Fixed by #1693
Assignees

Comments

@mwetter
Copy link
Contributor

mwetter commented Feb 3, 2023

This issue is to correct the circular parameter assignment reported in lbl-srg/modelica-buildings#3226

@FWuellhorst
Copy link
Contributor

From my side, there is another issue with regard to use_eta_Carnot_nominal:

If false, etaCarnot_nominal is used to calculate COP_nominal, but disabled in the annotation. This is misleading.
Two options:

  1. Set COP_nominal to some fixed value, same as etaCarnot_nominal.
  2. Enable etaCarnot_nominal at any time to calculate COP_nominal. However, this would make the whole point of COP_nominal somehow redundant.
    I would prefer (1), as you either have a certain COP at nominal conditions at hand or not.

However, I don't know if this could introduce a non-backwards compatibility, as the default values change.

@mwetter
Copy link
Contributor Author

mwetter commented Feb 6, 2023

@FWuellhorst : Thanks for the suggestions. In the current implementation, I did not set COP_nominal to a fixed value as the value is highly depend on the temperatures. Rather, if use_eta_Carnot_nominal = false, the parameter COP_nominal is ignored (not used) because the model uses always the protected parameter etaCarnot_nominal_internal which is calculated as

  parameter Real etaCarnot_nominal_internal(unit="1")=
    if use_eta_Carnot_nominal
      then etaCarnot_nominal
      else COP_nominal/
           (TUseAct_nominal / (TCon_nominal + TAppCon_nominal - (TEva_nominal - TAppEva_nominal)))
    "Carnot effectiveness (=COP/COP_Carnot) used to compute COP";

Does this address your concern?

If so, I will see what default parameters can be removed in the examples to avoid duplicate bindings.

PS: I also tried to use the fixed attribute on eta_Carnot_nominal and COP_nominal but this then does not allow to provide a default value eta_Carnot_nominal = 0.3 in BaseClasses.Carnot which I think is useful to have.

@FWuellhorst
Copy link
Contributor

@mwetter: A fixed value of COP_nominal is not possible, I agree. If use_eta_Carnot_nominal=false, I would expect the value is not used in the model. However, it is then used to calculate the COP_nominal.
I would go for this option, just leaving it empty:
image

If users then know the COP, they can insert it.

Do unspecified parameters raise warnings in the latest Dymola and OpenModelica? If so, leaving the default equation is indeed best. But I would then leave the etaCarnot_nominal enabled, as it influences the COP_nominal.

mwetter added a commit that referenced this issue Feb 6, 2023
@mwetter
Copy link
Contributor Author

mwetter commented Feb 6, 2023

The GUI above would require to set fixed=not etaCarnot_nominal and then provide an initial equation

  if use_eta_Carnot_nominal then
    COP_nominal = etaCarnot_nominal *TUseAct_nominal / (TCon_nominal + TAppCon_nominal - (TEva_nominal-TAppEva_nominal));
  end if;

However, with such an implementation, if a user already set a value for COP_nominal, then this would be an error. So, I think we need to keep the default assignment

parameter Real COP_nominal(unit="1") = etaCarnot_nominal*TUseAct_nominal/
    (TCon_nominal+TAppCon_nominal - (TEva_nominal-TAppEva_nominal))

and clarify this in the info section which I did in 6225c93. Is this clear or do you have another resolution?

@FWuellhorst
Copy link
Contributor

Ok, then it's clear to me. Thanks for the documentation update.

mwetter added a commit that referenced this issue Feb 9, 2023
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 a pull request may close this issue.

2 participants