search

ibpsa / modelica-ibpsa-mpc

Model library for MPC
Other
8 stars 1 forks source link

removed checkBoundary for #1 #3

Closed Mathadon closed 4 years ago

Mathadon commented 5 years ago

this closes #1

Mathadon commented 5 years ago

@dhblum this PR adds IbpsaMpc/Resources/Scripts/merge.py , which merges the files from this PR. I made no manual adjustments. This PR now merges IBPSA.Media.Air and a subset of IBPSA.Fluid.Sources. Can you review this?

dhblum commented 5 years ago

Thanks @Mathadon Yes I will review.

Mathadon commented 5 years ago

@dhblum when this is merged I'll look at further PRs!

dhblum commented 5 years ago

@Mathadon Should Examples be added for these models in this PR? If so, should appropriate ones be added with the merge script (e.g. IBPSA.Fluid.Sources.Examples.PropertySource_T without temperature sensors and IBPSA.Fluid.Sources.Examples.PropertySource_h)? Or other custom added? Ultimately, these would be used for unit testing in https://github.com/ibpsa/modelica-ibpsa-mpc/issues/5.

Mathadon commented 5 years ago

Missing dependencies are a bit of an issue here. Also for plot scripts/unit tests. So I would not add examples by default. Their main function is to test the models, which is already done in IBPSA. We can add new examples later. Ok?

mwetter commented 5 years ago

My 2 cents is to add examples as we go along. Although models are tested in IBPSA, some are being modified for IbpsaMpc, and it is not clear a priori if such modifications break code.

dhblum commented 4 years ago

@Mathadon I'm ok developing a formal set of Example models along with https://github.com/ibpsa/modelica-ibpsa-mpc/issues/5 and getting this PR merged to be able to build off of. But trying to compile a simple model with just IbpsaMpc.Fluid.Sources.Boundary_pT fails because the package IbpsaMpc.Utilities does not exist, which is used in IbpsaMpc.Media.Air. Do you still mean for these models to compile? I think the Utilities package needs to be added to the merge script for this? Or is that what you meant with "Missing dependencies are a bit of an issue here?"

Also, I think https://github.com/ibpsa/modelica-ibpsa-mpc/pull/3/commits/728d2de7aca719a79bd1f4eb7a84c9544acbf159 overwrites what is done in https://github.com/ibpsa/modelica-ibpsa-mpc/pull/3/commits/38b4798432328a56fd6c6d1e316a7842badb394a, correct?

Mathadon commented 4 years ago

@dhblum I have added Utilities.Psychrometrics.Constants to fix the compilation issue. I'll need to add the third merge option for being able to deal with the second issue that you pointed out. I'll look into it.

Mathadon commented 4 years ago

@dhblum this is ready to be merged!

dhblum commented 4 years ago

@Mathadon Thanks so much. This is on my list this week to review!

Mathadon commented 4 years ago

👍

dhblum commented 4 years ago

I can compile and optimize a model that includes all source models in this PR. The implementation looks good to me.

One further question is what to do about documentation. All documentation, including revisions, are directly copied from IBPSA. Some of the documentation may not apply to the MPC library at the current moment, or possibly ever. For instance, the Fluid package UserGuide references heat exchangers and chiller models which are not yet implemented. Also, there may be restrictions on the use of models or parameters that are not reflected in the documentation from IBPSA.

Mathadon commented 4 years ago

We can either remove the documentation altogether, or keep it and use merge option 3 to modify the documentation if needed?

dhblum commented 4 years ago

I think it will be important to have some documentation, so perhaps merge option 3 will be the way to go as needed.

Mathadon commented 4 years ago

Does the current PR require changes or can we continue with this?