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
Comments
I think this is the boldest and most heroic issue in Annex 60 history! |
I wouldn't attempt it without having so many unit tests in place to check that it will be done correctly... |
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
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.
In #425 I did some further refactoring. In terms of functionality:
In terms of code:
Can you verify if you agree with such an implementation? I will then make desired changes and finalise documentation and revision history. |
@Mathadon 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 Also, note that
and pedantic check fails on many models. |
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 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 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! |
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
This is to see how to refactor the
.Movers
models toThe text was updated successfully, but these errors were encountered: