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

Add more detailed pressure exchanger #1264

Merged
merged 131 commits into from
Mar 26, 2024

Conversation

luohezhiming
Copy link
Contributor

Fixes/Resolves:

replace this with the issue #1262 fixed or resolved,

Summary/Motivation:

Add more details for the pressure exchanger that accounts for leakage, mixing, and the pressure differential

Changes proposed in this PR:

  • revise current pressure exchanger
  • add corresponding tests

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.


# Performance equations
@self.Constraint(self.flowsheet().config.time, doc="Pressure transfer")
def eq_pressure_transfer(b, t):
return (
b.low_pressure_side.deltaP[t]
== b.efficiency_pressure_exchanger[t] * -b.high_pressure_side.deltaP[t]
b.feed_side.deltaP[t]
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest implementing a more precise efficiency calculation method for PressureExchangeType.high_pressure_difference option, defining efficiency as the ratio of energy output to energy input. This approach effectively incorporates the effects of HPD and leakage into efficiency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ElmiraShamlou the current definition of efficiency in eq_pressure_transfer is exactly same as ratio of energy output to energy input. I've tried to directly define the efficiency based on the energy output/energy input and get exact same results. Though, it is not so evident, ΔP_FS=-ηΔP_BS is same as energy output/ energy input

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @luohezhiming . The current definition of efficiency is equivalent to energy ratios only when there's no leakage and the flow rates on both the feed and brine sides are the same. You got the same results from both methods because the leakage was either very small or zero. But if leakage increases, it will have a bigger effect on efficiency. We can include the leakage inefficiency in a complete energy equation that incorporates both pressure and flow rates.

Copy link
Contributor Author

@luohezhiming luohezhiming Mar 22, 2024

Choose a reason for hiding this comment

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

I redefine the efficiency as the ratio of energy_in over energy_out, but I find the solution results will change for the no_mass_transfer case. Since a lot of flowsheet use this basic pressure exchanger without mass transfer, @TimBartholomew , I want to confirm with you should we change this efficiency equation? If we do, then all flowsheets using the pressure exchanger will have different results.

> 1e-4
and not self.config.has_mass_transfer
and not self.config.has_mixing
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this error message actually helps with initialization. If we need to keep it, maybe we should change self.config.has_mixing to self.config.has_leakage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ElmiraShamlou I agree we also have to add has_leakage for this check, but we also need to include has_mixing because we introduce mass_transfer_term into the control volume when add mixing even though it might not affect the volumetric flowrate if there is no leakage, but the introduced mass_transfer_term might cause some minor errors. Note that in most cases without transfer, after initialization, we should have Q_FS_in=Q_BS_out and deactivate the equal_flow_vol constraint to make initialization work.

Copy link
Contributor

Choose a reason for hiding this comment

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

@luohezhiming please tag this in the issue for tracking any potential updates we may (or may not) want to make in in a future PR. For now, I think we can merge what you have for the March release and refine later, if needed.

@adam-a-a adam-a-a added the 🍒 Merge to rel Merge commit(s) from this PR to release branch label Mar 25, 2024
Copy link
Contributor

@ElmiraShamlou ElmiraShamlou left a comment

Choose a reason for hiding this comment

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

Approved. Great work!
Based on the team's decision, some of the requested changes can wait until future PRs.

Copy link
Contributor

@adam-a-a adam-a-a left a comment

Choose a reason for hiding this comment

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

LGTM - @luohezhiming please make an issue to track any potential improvements we might want to consider later for the detailed presssure exchanger. I have on or two that come to mind, so I will add after you start the issue.

@lbianchi-lbl lbianchi-lbl enabled auto-merge (squash) March 25, 2024 23:46
@lbianchi-lbl lbianchi-lbl merged commit ab9870b into watertap-org:main Mar 26, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍒 Merge to rel Merge commit(s) from this PR to release branch Priority:Normal Normal Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants