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

removed checkBoundary for #1 #3

Merged
merged 17 commits into from
Dec 10, 2019
Merged

Conversation

Mathadon
Copy link
Member

@Mathadon Mathadon commented Aug 14, 2019

this closes #1

@Mathadon
Copy link
Member Author

@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?

@Mathadon Mathadon mentioned this pull request Sep 10, 2019
@dhblum
Copy link
Collaborator

dhblum commented Sep 10, 2019

Thanks @Mathadon Yes I will review.

@dhblum dhblum self-requested a review September 10, 2019 14:35
@Mathadon
Copy link
Member Author

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

@dhblum
Copy link
Collaborator

dhblum commented Sep 26, 2019

@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 #5.

@Mathadon
Copy link
Member Author

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
Copy link
Collaborator

mwetter commented Sep 26, 2019

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
Copy link
Collaborator

dhblum commented Oct 29, 2019

@Mathadon I'm ok developing a formal set of Example models along with #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 728d2de overwrites what is done in 38b4798, correct?

@Mathadon
Copy link
Member Author

@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
Copy link
Member Author

@dhblum this is ready to be merged!

@dhblum
Copy link
Collaborator

dhblum commented Dec 3, 2019

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

@Mathadon
Copy link
Member Author

Mathadon commented Dec 4, 2019

👍

@dhblum dhblum mentioned this pull request Dec 4, 2019
@dhblum
Copy link
Collaborator

dhblum commented Dec 5, 2019

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
Copy link
Member Author

Mathadon commented Dec 5, 2019

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

@dhblum
Copy link
Collaborator

dhblum commented Dec 6, 2019

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

@Mathadon
Copy link
Member Author

Mathadon commented Dec 9, 2019

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

Copy link
Collaborator

@dhblum dhblum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is ready to merge.

@Mathadon Mathadon merged commit 3a23b1a into master Dec 10, 2019
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 this pull request may close these issues.

sources
3 participants