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

Move plugflowpipe to baseclasses #1537

Merged
merged 8 commits into from
Oct 29, 2021
Merged

Move plugflowpipe to baseclasses #1537

merged 8 commits into from
Oct 29, 2021

Conversation

bravache
Copy link
Contributor

The main change of this PR is to move the content of IBPSA/Fluid/FixedResistances/PlugFlowPipe.mo to IBPSA/Fluid/FixedResistances/BaseClasses/PlugFlowPipe.mo and have the internal pipe resistance element res replaceable.
The exposed class has the same behavior as previously, and the baseclass can be reused to replace the pipe resistance with a LosslessPipe.

@bravache
Copy link
Contributor Author

@mwetter If all test passes, this is ready to merge. This brings in the MBL changes to IBPSA.

@mwetter
Copy link
Contributor

mwetter commented Oct 16, 2021 via email

@bravache
Copy link
Contributor Author

@baptiste Ravache @.> : do you also want to add the discretized plug flow pipe or is there a reason to omit it?

On Fri, Oct 15, 2021, 11:27 AM Baptiste Ravache @.
> wrote: @mwetter https://github.com/mwetter If all test passes, this is ready to merge. This brings in the MBL changes to IBPSA. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1537 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKEPOGR3YV332ZHGHCC2HTUHBXCFANCNFSM5GCQXI6Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

@mwetter : No reason, except that I assumed that the level of scrutiny for merging the PlugFlowPipeDiscretized would be higher, and didn't want it to hold the deployment of this branch. Also, from previous conversation, I got the idea that the discretized pipe would be a class specific to the MBL and not be included in IBPSA, but I might have misunderstood.

Do you recommend I add it now? Would I need to restart the panel review?

@bravache bravache self-assigned this Oct 20, 2021
@bravache bravache requested a review from mwetter October 20, 2021 16:12
Copy link
Contributor

@mwetter mwetter left a comment

Choose a reason for hiding this comment

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

This is ready to merge from my point of view.

@mwetter
Copy link
Contributor

mwetter commented Oct 25, 2021

@Mathadon : Can you please review so we have a non-LBL reviewer.

@mwetter mwetter requested a review from Mathadon October 25, 2021 21:13
@Mathadon
Copy link
Member

I'm just wondering whether IBPSA/Fluid/FixedResistances/PlugFlowPipe.mo needs a new revision history? The old revision history has been copied to the base class so this 'new' model could use a new revision history.

@mwetter
Copy link
Contributor

mwetter commented Oct 26, 2021

@Mathadon : Thanks, I updated the revision history and will merge it after the tests pass.

@mwetter
Copy link
Contributor

mwetter commented Oct 26, 2021

This closes #1538

@mwetter mwetter merged commit 3c8caaf into master Oct 29, 2021
@mwetter mwetter deleted the merge_plugflowpipe_edit branch October 29, 2021 11:24
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