-
Notifications
You must be signed in to change notification settings - Fork 155
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
Using records for providing pump parameters #209
Comments
Hi Filip I think using records would be a good addition for the
where I made a development branch https://github.com/lbl-srg/modelica-buildings/tree/issue209_pumpRecords, please make any pull request to this branch so we can iterate on the implementation before putting it on the master. We did a similar implementation in |
@Mathadon reported:
I do not know a solution for this. I'll add more examples based on any feedback. Also, I suppose an example similar to issue #202 is in order? |
I looked at your pull request #211. I merged it to the development branch issue209_pumpRecords (only final changes are merged to the master). |
Woops, sorry for the wrong pull request. I don't think the power record can be removed because you still need it if you want to override an individual parameter. |
This is for #209. Merged pull requests to development branch. Changed MoverData.mo to Generic.mo as the data package is already a sub-package of Movers.
This is for #209. Now, performance data are only declared once, in the record called data. This record has been renamed from moverData to data because it is instantiated in the mover model, hence we can drop the name Mover. All unit tests give the same results.
This is for #209. The change was made to use a naming that is consistent with the chiller and the solar collector models, and also because data is too generic a name.
This is for #209. The change was made to use a naming that is consistent with the chiller and the solar collector models, and also because data is too generic a name.
This is for #209. The change was made to use a naming that is consistent with the chiller and the solar collector models, and also because data is too generic a name.
@Mathadon : Looking at
Wouldn't it be better to remove N_max, and set N_nominal=2800? Where do the N_nominal=1800 come from? |
@mwetter I agree that this is counter-intuitive. The reason is that most pump curves are 'interrupted' by the boundary you can see on the graph. To be able to apply similarity laws to the curve it must be 'complete'. The first curve that is not interrupted is the curve of 1800 rpm. Setting N_nominal to N_max would mean that the pump curves needs to be rescaled manually or that the pump model handles this properly. Note that for this pump this is a very extreme example with a big difference between N_nominal and N_max. I have documented Buildings.Fluid.Movers.Data.Pumps.Stratos25slash1to6 where this difference is less extreme. In that model I also explain why that curve is chosen. Is it necessary to add the datasheets as a pdf to the library? |
@Mathadon : Please don't include the pdfs of the data sheets, as these may be copyright by the manufacturer and we may not be allowed to distribute them. Regarding the |
I do not know how this is usually handled. I googled some other brands/types: Note that it is not power consumption that is limited as this would result in a hyperbolic curve. A maximum current may be more logical since thermal losses are proportional to the square of the current. Another way to handle this would be by adding a 'maximum load curve' in the model. |
It looks like we should add an optional maximum pressure raise curve to the pump data, and then restrict in
reproduces the maximum pressure raise specified by the parameter. This change also requires modifications to |
That indeed sounds like the most robust solution. Do you want to implement this as a separate issue or continue working in #209 ? |
It's probably best to make a separate branch for this that branches off from issue209_pumpRecords. That will make it easier to keep track of the changes. |
Can the current issue be merged before continuing with the maximum pressure raise curve? |
I suggest to make a new branch off from |
Hi Michael, FlowMachine_y currently contains the following code:
When using the same 'per' record for a FlowMachine_y pump as for a FlowMachine_Nrpm pump this final N_nominal generates a warning since the per record already contains a value for N_nominal.
I suggest to remove the 'final' or remove the default value altogether and since it is already defined in Buildings.Fluid.Movers.Data.Generic? Additionally the model contains:
I think the 'max' value should not be equal to 1 since the pump characteristic may be defined for a different rpm than the maximum rpm. I suggest to either:
If you agree I can implement these changes if you like. |
Hi Filip
to
and set a value for I agree that the I would rather use |
Ok, I'll implement this. Another issue: I get the following warning when checking a model using the new pumps:
Changing the 'parameters' in the 'generic' pump/fan records to 'constants' removes this warning. To remove the warning it is only necessary to make the pressure curve a constant. However I assume all parameters in the generic record can be set to constants? |
The efficiency currently only depends on r_V. I therefore don't understand Setting V_flow_max = max(pressure.V_flow) is not good. I suggest to On Tue, Sep 30, 2014 at 11:25 PM, Filip notifications@github.com wrote:
|
r_V is a function of V_flow_max, which is (or will be) a function of the pressure curve. Therefore changing the pressure curve will change the efficiency of the pump.
However, it is not known in the mover record, where I would like to calculate the efficiency.
Yes indeed, that's what I did:
This implementation works, but uses V_flow instead of r_V since V_flow_max is not known in the record. Using r_V is impossible without moving it over. Having elaborate discussions in an issue may indeed not be the best way to proceed. However writing reports is time-consuming and in this case I have provided a working implementation so I hope this can serve as sufficient grounds for discussion. Feel free to make changes directly in the code if you do not agree with some details? |
Actually Buildings.Fluid.Movers.FlowMachine_dp2 seems to have some problems when switching in a discrete way: the non-linear iteration for r_N diverges due to the bad initial guess. I think I know how to solve this but it is probably best to keep this until other things are sorted out. |
This summarizes todays web-conference with @Mathadon:
This should allow scaling the fan/pump to a larger or smaller volume flow rate so that if
|
I removed them. |
I addressed points 1 through 5 except for any changes involving documentation in point 2. |
For #209. This restructuring is required to avoid that mover data require the user to enter data that are not used by the model. Using default values such as V_flow={0} does not work as this lead to a division by zero in some model. Also, not declaring a default value also does not work as this led to error such as 'Failed to expand the variable floMacSta.per.pressure.V_flow'. Therefore, the package has been redesigned so that only the minimum set of required data need to be entered. See also the updated user's guide.
This was needed because the efficiency function of the movers can no longer be used.
@Mathadon : |
@mwetter I used that model to generate the picture below: this picture can serve as a validation for the pump model. If you include it in the library then this model may be a good reference to show where the result came from? If this is not of interest then feel free to remove it! |
@Mathadon Can you please include the figure in the example and document the example (see my previous note). This would be useful for users. |
Of course, no problem |
The background of the figure above is from a data sheet. You mentioned before that this may be a problem (copyright/IP issue?). Is this still the case? |
I think for this case it is OK. Best would be to write "Figure adapted from On Wed, Nov 26, 2014 at 10:09 AM, Filip notifications@github.com wrote:
|
@Mathadon : Can you please add the documentation regarding I also move some examples to the This is now close to complete so that we can move it to the master and also the These are the renames I did:
|
I think I addressed the documentation in #322 ? |
This is now on the master branch |
Hi Michael,
during the Annex60 meeting we discussed the possibility for using records to insert the parameters of a pump model. This way no documentation is 'lost' when extending a model. Also it makes switching between pumps easier.
I have made an implementation for this but it still needs some cleaning of the code. If you still think this is a good idea I'll put some work into it and make a pull request?
Best regards,
Filip
The text was updated successfully, but these errors were encountered: