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

refactor .Movers to use blocks rather than multiple inheritance #417

Closed
mwetter opened this issue Feb 19, 2016 · 5 comments · Fixed by #420
Closed

refactor .Movers to use blocks rather than multiple inheritance #417

mwetter opened this issue Feb 19, 2016 · 5 comments · Fixed by #420
Assignees

Comments

@mwetter
Copy link
Contributor

mwetter commented Feb 19, 2016

This is to see how to refactor the .Movers models to

  • avoid multiple inheritance of partial classes
  • conform to the coding convention to not mix textual and graphical equations in the same class
@mwetter mwetter self-assigned this Feb 19, 2016
mwetter added a commit that referenced this issue Feb 19, 2016
@Mathadon
Copy link
Member

I think this is the boldest and most heroic issue in Annex 60 history!

@mwetter
Copy link
Contributor Author

mwetter commented Feb 19, 2016

I wouldn't attempt it without having so many unit tests in place to check that it will be done correctly...

mwetter added a commit that referenced this issue Feb 19, 2016
Reference results changed slightly, mainly because the mass flow sensor is now at the inlet as opposed to the outlet. This gives also smaller nonlinear system of equations
@mwetter mwetter changed the title refactor .Movers to use blocks rather than mulitle inheritance refactor .Movers to use blocks rather than multiple inheritance Feb 20, 2016
mwetter added a commit that referenced this issue Feb 21, 2016
Nonlinear systems of equations got smaller.
Only small changes in flow rate because mass flow rate at inlet of fan
rather than at outlet is used to compute the fan performance.
@Mathadon
Copy link
Member

In #425 I did some further refactoring.

In terms of functionality:

  • we can now also compute the electrical power consumption correctly when using FlowControlled_* movers

In terms of code:

  • I integrated FlowMachineInterface and EfficiencyInterface, which were therefore moved into PartialFlowMachine
  • I removed the FlowMachine record since it only contains efficiency parameters. These parameters were used in FlowMachine_m_flow and in FlowMachine_dp, but since these models did not allow to correctly compute the efficiency using these efficiency curves it does not makes sense to provide this option, since the results will be wrong. Instead FlowMachine_* now also uses Annex60.Fluid.Movers.Data.SpeedControlled_y. If no detailed pump curves are available to the user, then he may use new parameters that allow to set a fixed efficiency eta and motorCooledByLiquid. I.e. eta is not a function of the flow rate. Only then the power computation is correct for all values of (dp,m_flow). The user can choose between the detailed computation using pump curves, or the simplified implementation using parameter use_record.
  • Unit tests produce the same results, except Annex60.Fluid.Movers.Validation.PowerSimplified since I changed the example. One other example has a larger number of non-linear algebraic loops during initialisation.
  • I added a unit test Annex60.Fluid.Movers.Validation.PowerExact that demonstrates that all three movers produce the same electrical power output value.
  • Annex60.Fluid.Movers.BaseClasses.FlowMachineInterface has been reworked with conditional inputs/outputs such that the model correctly computes the outputs depending on which variables are prescribed.

Can you verify if you agree with such an implementation? I will then make desired changes and finalise documentation and revision history.

@mwetter
Copy link
Contributor Author

mwetter commented Feb 25, 2016

@Mathadon
What are you doing in FlowMachine_* if SpeedControlled_y is used, but the input signal for dp or m_flow is such that the operating point is not on the pump curve? I think your approach is currently flawed as it can give combinations of (dp, V_flow) that are outside the valid regions. Or do you adjust the speed then? Please explain in the documentation so I (and others) understand the model.

I also think the efficiency should ALWAYS be in the data record. It is in my view confusing to a user to find efficiency specification in the model, but also in records, and then the user has to make sure he sets it at the right place and sets the flag that overwrites which one is used. This reminds me to the refactor of dynamicBalance that we just did in which two parameters were used for similar declaration. I see no problem for keeping Fluid.Movers.Data.FlowControlled and if a user wants to use the pressure curves to compute speed and then efficiency (I think this is what you do but I am not sure due to lack of documentation) then Fluid.Movers.FlowControlled_dp uses Fluid.Movers.Data.SpeedControlled_y. As one record is a subclass of the other, this should work. But you need to make sure that a solution always exists which I am not sure it is the case (due to the lack of explanation in the documentation).

Also, note that

*** Warning: MoverParameter.mat: Translation statistics for initialization changed for nonlinear, but results are unchanged.
 Old = 1, 2, 1, 1, 1, 2, 2, 2
 New = 1, 2, 2, 1, 2, 1, 2, 1, 2

and pedantic check fails on many models.

@Mathadon
Copy link
Member

Mathadon commented Mar 1, 2016

I do indeed adjust the speed accordingly. This introduces a non-linear algebraic loop of 1 variable after reduction if this option is used, which is not the case by default.
I prefer to update the documentation once we agree on an implementation. I think I clearly described my changes above.

I can see why you would want that, but with the old implementation this means that the efficiency is not computed correctly since the pressure curves are not used to determine the correct operation point. I think this is a more important downside. An alternative implementation would be to have a record that contains a constants efficiency, since in this case the results are correct because then the efficiency is not a function of the pressure curve. However this would mean we have a record for 2-3 variables and quite some hassle in the implementation of the models. I think my suggestion is, overall, more elegant.

I tried your suggestion to keep Fluid.Movers.Data.FlowControlled in the mover models and to redeclare it with Fluid.Movers.Data.SpeedControlled_y when needed, but this does not work: the variables of per (type SpeedControlled_y) need to be propagated into eff.per and this is not possible when per is by default of type FlowControlled, since the variables then do not exist in this record.
The alternative would be to define two records in Movers.FlowControlled, but this was in my opinion a bigger mess than adding 3 new variables. A second alternative would be to use the same record type for FlowControlled and SpeedControlled_y.

To conclude: I see why you disagree with some of the changes, but since the added functionality is very nice to have, I think it's worth looking into this a bit further and get this sorted out!

Mathadon added a commit to Mathadon/modelica-annex60 that referenced this issue Mar 2, 2016
FlowControlled and SpeedControlled are now integrated into PartialFlowMachine
alle movers use the same record per, which is an instance of Generic, which is equivalent
to the old SpeedControlled_Nrpm

updated documentation and cleaned up connections
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