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.

Sign up for GitHub

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

Jump to bottom

New (and updated) Airflow Classes #1437

Closed
wants to merge 25 commits into from
Closed

New (and updated) Airflow Classes #1437

wants to merge 25 commits into from

Conversation

kldjonge
Copy link
Contributor

@kldjonge kldjonge commented Dec 14, 2020 •
edited
Loading

New components and related baseclasses were developed for modelling multizone airflow.

@kldjonge
New and updated Multizone Baseclasses
22dde74
@kldjonge
New and updated 1-way-flow airflow components
1823d32
@kldjonge
IDEAL prescribed mover and Outside_CpData
08612d1
@kldjonge
Suitable component names and package order
cb0891e 
Updated the component names of badly names components and changed the package order. Also updated some minor things on the documentation
@MassimoCimmino
Copy link
Contributor

@kldjonge Is the pull request up to date? As I mentioned on the last WP1.1 call, I can make a first review.

@kldjonge
Copy link
Contributor Author

It is up-to-date. A first review would be great!

@MassimoCimmino
Copy link
Contributor

@kldjonge I made a quick review of the commits on the pull request and annotated the code on my fork.

The "biggest" issues I find are the calls to spline interpolation in IBPSA.Airflow.Multizone.BaseClasses.flowElementData in and IBPSA.Airflow.Multizone.BaseClasses.windPressureProfile, and the missing test cases for the new models. I suggest to uniformize the naming of models and variables (e.g. PowerLaw vs Powerlaw, and _MassFlow vs _mflow vs _m_flow), and to update the user's guide (IBPSA.Airflow.Multizone.UsersGuide) as it will become out-of-date.

@kldjonge
Copy link
Contributor Author

Many thanks for the review. Looks like it is mainly small changes and naming issues/inconsistencies.

Concerning the splines I will have to rethink the implementation a little then. I tried to not have the derivatives (and eg. the addition of the previous and last point) at top level for clarity/readability of the models. Any suggestions on how to implement it alternatively to avoid redoing the calculation of the derivatives every time step but stil not clutter the top-level model to much with these 'background' calculations?

@kldjonge
Copy link
Contributor Author

@mwetter, does is not make sense to allow this merge into /~https://github.com/ibpsa/modelica-ibpsa/tree/issue1436_AirflowNewClasses already?

For review purposes it seems more logical that eg. Massimo can edit in the same repository. Now, changes were made on his fork and I cherry picked them to mine to link it back to this pull request. If this is the typical way of working it no problem off course, just wondering if this is the most efficient workflow.

@MassimoCimmino
Copy link
Contributor

I don't think the initialization of derivatives would clutter the TableData_ models since the interpolation is the only operation at the top level.

@mwetter
Copy link
Contributor

mwetter commented Mar 26, 2021

@kldjonge @MassimoCimmino : That's fine to merge into this development branch, which I will do now.

kldjonge added 12 commits April 6, 2021 10:05
@kldjonge
Changed names in Multizone package and fixes
17da543 
Changed name of PowerLawResistance to PartialPowerLawResistance and likewise for PowerLawResistance_mflow. Also changed MassFlow and VolumeFlow to m_flow and V_flow.

Fixes for:
IBPSA/Airflow/Multizone/BaseClasses/PartialOneWayFlowelement.mo
IBPSA/Airflow/Multizone/BaseClasses/PowerLawResistance.mo
IBPSA/Airflow/Multizone/BaseClasses/PowerLawResistance_mflow.mo
@kldjonge
Deleted PrescibedFlow
a1b235c 
Addition of this mover is out of scope for the present development branch.
@kldjonge
Fixes for various flow elements
2a7212d
@kldjonge
set missing default m_flow_nominal for TableData models
5a88acc 
Nominal flow rate derived from table data as smallest flow found in the table.
@kldjonge
Updated m_flow_nominal for powerlaw airflow models.
68eb9a1 
Changed this value back to original default (It was changed do 'small', but that is not necessary for common use of the components)
@kldjonge
Validation case with embedded CONTAM reference results
aad4cac 
Testbed for flow elements in the range -50Pa to +50Pa.
CONTAM results are provided at fixed pressure differences as reference data.
@kldjonge
Outside_CpData (fixes and example)
c366ba0
@kldjonge
Updates to annotation
c7e4c6b
@kldjonge
Examples for flowElementData and windPressureProfile functions
3e6a16d
@kldjonge
Fixes for flowElementData and windPressureProfile
1160e3d
@kldjonge
Updated userguide and revision history
ac239a6
@kldjonge
Cleaned up models where fix was unnecessary
223239a 
Answered some of the questions Massimo left as a 'fixme', this commits cleans these up but also provides an overview of the answers.
@kldjonge
Copy link
Contributor Author

kldjonge commented Apr 6, 2021

@MassimoCimmino , I incorporated your proposed fixes. I also updated some of the annotation and made some minor enhancements to some models along the way.

For the validation of the models, I made a CONTAM model as testdata for the one way flow elements and embedded the data:
IBPSA_FlowElement_Data.xlsx
At the actual datapoints, the data almost perfectly corresponds. In modelica, looking at the data, it looks like there might be a slight difference, but that is mainly due to the interpolation of the testdata.

@kldjonge kldjonge requested a review from MassimoCimmino April 6, 2021 21:41
@kldjonge kldjonge self-assigned this Apr 6, 2021
mwetter added a commit that referenced this pull request Apr 29, 2021
@mwetter
Changed constrainedBy for #1437
e1442ec
kldjonge added 3 commits May 3, 2021 18:56
@kldjonge
Updated Outside_CpData and it's example
dbca4e7
@kldjonge
Cleaned up OneWayFlow validation case
c13be0d
@kldjonge
Optional: script for airflow element validation
3008208 
Please delete if not usefull.

Script that generates a graph to compare CONTAM(test data) and Modelica (simulated data) with dP as independent variable.
@kldjonge kldjonge requested a review from mwetter May 3, 2021 17:06
@kldjonge
Copy link
Contributor Author

kldjonge commented May 3, 2021 •
edited
Loading

@mwetter, shouldn't we move from my fork to the IBPSA development branch (issue1436_AirflowNewClasses) already to make the reviewing easier?

@kldjonge kldjonge linked an issue May 3, 2021 that may be closed by this pull request
Branch for extended multi-zone airflow package #1436
Closed
@mwetter
Copy link
Contributor

mwetter commented Jun 11, 2021

@mwetter, shouldn't we move from my fork to the IBPSA development branch (issue1436_AirflowNewClasses) already to make the reviewing easier?

Since @Mathadon will do the next review I will leave it up to him to merge on a development branch to avoid that I interfere with his revisions/edits.

@Mathadon
Copy link
Member

👍

@kldjonge
Copy link
Contributor Author

This PR is replaced by #1500

@kldjonge kldjonge closed this Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MassimoCimmino MassimoCimmino Awaiting requested review from MassimoCimmino

@mwetter mwetter Awaiting requested review from mwetter

Assignees

@kldjonge kldjonge

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Branch for extended multi-zone airflow package
4 participants
@kldjonge @MassimoCimmino @mwetter @Mathadon