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

Use mblv9 #454

Merged
merged 52 commits into from
May 25, 2022
Merged

Use mblv9 #454

merged 52 commits into from
May 25, 2022

Conversation

vtnate
Copy link
Contributor

@vtnate vtnate commented Apr 26, 2022

Any background context you want to provide?

The GMT should work with mainline MBL, to enable new capabilities as they are developed

Until we have a compiler for MSLv4, success for this PR will be defined as:

  • the manual test procedure working no matter which model you open
  • all other pytests passing (as of 2022-05-11 this is 18 failing, 78 passing)

What does this PR accomplish?

Update templates, code, and test files to work with MBLv9

How should this be manually tested?

  • Check out MBL master branch
  • poetry run py.test
  • open a model in Dymola
  • Simulate the model
  • Bask in the glow of a simulated model without errors 🎉

What are the relevant tickets?

#409

@vtnate vtnate marked this pull request as ready for review May 11, 2022 20:43
@vtnate vtnate requested review from nllong and amyeallen1 May 11, 2022 20:43
@vtnate
Copy link
Contributor Author

vtnate commented May 16, 2022

@nllong The failing test has a Poetry deprecation warning. I don't know if that is related to the check failure or not. Can you check sometime at your convenience? I don't think it's urgent.

Copy link

@amyeallen1 amyeallen1 left a comment

Choose a reason for hiding this comment

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

This looks great! Wonderful job, Nate! I ran the tests and replicated your results in terms of the # passing and failing, and ran the CHP system, district htg & clg with Teaser, district cooling (time series), and solar PV models in Dymola. They generally performed as expected, with a few specific notes below about things that are beyond the scope of this PR:

Regarding a few specific points:

  • CHP model: The high HHW supply water temperatures seem to happen for short periods of time at conditions of low- or no mass flow, so I agree that it isn’t a problem. I noticed a possible area for future investigation in terms of the performance of the CHP model, and I'll submit an issue. (When I ran the simulation for a 2-month period, I noticed quite a few periods of negative mass flow at the supply temperature sensor (also corresponding to negative mass flow at the return mass flow rate sensor). )
  • This branch is incorporating changes that will address the inconsistency in weather files between the CHW plant and building load models (such as Teaser) that currently occurs in the models generated by the tests: https://github.com/urbanopt/geojson-modelica-translator/tree/schema_mod_weather_file

@nllong nllong force-pushed the use-mblv9 branch 3 times, most recently from d51ef6d to 46d825d Compare May 20, 2022 03:05
@vtnate
Copy link
Contributor Author

vtnate commented May 20, 2022

@nllong Thank you for fixing the CI workflow! I could use some education on CI at some point in my career, it continues to be a mystery to me in general.

Local tests all pass when the not msl_v4_simulation message is added to pytest. With the excellent review by @amyeallen1 I am confident that the models run as expected in Dymola.

@amyeallen1
Copy link

Aww, thanks, Nate! Wonderful job! @vtnate

nllong and others added 6 commits May 24, 2022 09:03
Co-authored-by: Nathan Moore <nathan.moore@nrel.gov>
Co-authored-by: Nathan Moore <nathan.moore@nrel.gov>
Break out build and simulate tests
@nllong
Copy link
Member

nllong commented May 24, 2022

@vtnate -- any reason not to merge once these tests pass?

@vtnate
Copy link
Contributor Author

vtnate commented May 25, 2022

Nope, let's do it! 🚀

@nllong nllong merged commit f9fd778 into develop May 25, 2022
@nllong nllong deleted the use-mblv9 branch May 25, 2022 14:17
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.

3 participants