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

New, staged pump records #396

Closed
Mathadon opened this issue Jan 19, 2016 · 7 comments
Closed

New, staged pump records #396

Mathadon opened this issue Jan 19, 2016 · 7 comments

Comments

@Mathadon
Copy link
Member

I have some records for the Wilo TOP-S series (http://productfinder.wilo.com/en/COM/productrange/000000090002c94500020023/fc_range_description?lang=en&iwp%5B%5D=powertree-R-0).

These are staged pumps and it seems we have not foreseen a proper way of handling this in Annex60.Fluid.Movers.Data.SpeedControlled_Nrpm. In my opinion it should contain:

  parameter Real[:] speeds(each final unit="1/min") = {0} 
    "Vector of speed set points when using stages";

since it's a property of the pump.

I propose to add this so I can add the records for these pumps correctly.

I'm not sure if we should also add

  parameter Real[:] normalized_speeds(each final unit="1") = {0} 
    "Vector of normalized speed set points when using stages"
    annotation(Dialog(enable=inputType == Annex60.Fluid.Types.InputType.Stages));

to Annex60.Fluid.Movers.Data.SpeedControlled_y.

I'll already make a pull request without this.

Mathadon added a commit that referenced this issue Jan 19, 2016
added parameter in SpeedControlled_Nrpm to allow this
changed default value in SpeedControlled_Nrpm

this is for #396
@mwetter
Copy link
Contributor

mwetter commented Jan 19, 2016

I have the following comments in addition to the comments in the pull request (https://github.com/iea-annex60/modelica-annex60/pull/397/files)

  • In SpeedControlled_Nrpm, the parameter N_nominal and speeds (which should be called N) should be assigned from the record per as they are a property of the pump.
  • The same should be done for SpeedControlled_y with normalized_speed and normalized_speeds, which need to be added to the record definition and given default values. Should we rather call this n_nominal and n to indicate that it is dimensionless? normalized_speed does not indicate anything about being for the nominal conditions and hence it looks like something different from N_nominal. Since Data.SpeedControlled_Nrpm extends Data.SpeedControlled_y, it needs to assign n with the final keyword so that these entries are consistent.

@Mathadon
Copy link
Member Author

Regarding your second suggestion: if we add parameter n_nominal then I think we should change the implementation of speedControlled_y since it currently assumes that the speed/efficiency/power curve is defined for n_nominal=1. The user may/should interpret the value of n_nominal as the normalised speed for which he/she provides the pump curves and not changing it may lead to wrong model use. Do you agree we should add this too?

@mwetter
Copy link
Contributor

mwetter commented Jan 20, 2016

I suggest to

  • make a list in the user guide that shows the different parameters such as speed and speeds. This will also help us to verify that a consistent naming is used for the four models.

Revised: this is in my view not needed as the selected parameters are clear.

mwetter added a commit that referenced this issue Feb 18, 2016
mwetter added a commit that referenced this issue Feb 18, 2016
@mwetter
Copy link
Contributor

mwetter commented Feb 18, 2016

@Mathadon I refactored the models, in particular the choice of parameter names which are now speed or speed_rpm, and speeds and speeds_rpm. Can you please have a look at it?

I also removed {y,N,m_flow,dp}_filtered as I don't see there is any use for these outputs. One can get their values from the output signal {y,N,m_flow,dp}_actual.

For SpeedControlled_{y,Nrpm}, I moved all performance data, also for the stages, to the performance data record.
For FlowControlled_{dp,m_flow}, I left the constant head (or flow) and the values for the stages in the model as these are artificial values which don't really belong into any (product-specific) performance record.

I also update the performance data for the Wilo pumps due to the new definition of the record, e.g., the nominal speed is automatically taken from the highest stage.

I get different results for the regression tests for MoverParameters, MoverContinuous and MoverStages. After my revision 0e65a37, these regression tests now have the same mass flow rate for SpeedControlled_y and SpeedControlled_Nrpm, while they had different values before. I think the new results are correct and the old are wrong. Can you please verify. I have not yet committed the new results.

Please let me know if you agree to these changes, in which case I can make a pull request.

@Mathadon
Copy link
Member Author

@mwetter I agree with

  • the new parameter names,
  • the removal of _filtered outputs,
  • moving performance data into the records,
  • leaving the artificial parameter values in the mover models.

The different regression test results are probably caused by the fact that you assign speed_rpm_nominal= speeds_rpm[size(speeds_rpm, 1)]. Since in MoverParameter you redeclare redeclare Annex60.Fluid.Movers.Data.Pumps.Wilo.Stratos25slash1to4 per(speeds_rpm={0,1000,2000}, this changes the pump speed for which the pressure() record is defined, which leads to wrong results.
My suggestion would be to require speed_rpm_nominal to be set in the performance records and to set speeds_rpm={speed_rpm_nominal} by default. I'll verify if that solves the problem and then make a pull request!

Note also that defining motorEfficiency and hydraulicEfficiency inFlowControlled may be misleading since a proper implementation of similarity laws requires the pressure curve and nominal speed to be incorporated in the efficiency calculation. But I think we established before that we accept this error.

@mwetter
Copy link
Contributor

mwetter commented Feb 18, 2016

In 7f331dd I added speed_nominal and speed_rpm_nominal back to the data record, and use them in gaiSpe to normalized the input signal.
I also use them for the default values for speeds and constant_speed.

All unit tests now produce the same results as before the change.

@Mathadon
Copy link
Member Author

Fixed in #397

Thanks!

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

No branches or pull requests

2 participants