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

Added bipolar membrane unit model and costing #1500

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

johnson12742
Copy link

@johnson12742 johnson12742 commented Oct 2, 2024

Fixes/Resolves:

(replace this with the issue # fixed or resolved, if no issue exists then a brief statement of what this PR does)

Summary/Motivation:

Adding the bipolar membrane unit model capability

Changes proposed in this PR:

  • Bipolar Electrodialysis 0-D unit model with a test. Heavily borrows from Electrodialysis 0-D.
  • Documentation for the Bipolar Electrodialysis 0-D unit model
  • Costing package for Bipolar Electrodialysis unit model. This is a slightly modified version of the electrodialysis costing. With bipolar there is a negative cost component (value addition)

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.

@johnson12742 johnson12742 marked this pull request as ready for review October 3, 2024 00:59
@lbianchi-lbl lbianchi-lbl added the Priority:Normal Normal Priority Issue or PR label Oct 3, 2024
Figure 1. Schematic representation of a bipolar electrodialysis unit


One bipolar membrane along with the **Acidate** and **Basate** channels can thus be treated as a modelling unit that can
Copy link
Contributor

Choose a reason for hiding this comment

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

Are acidate and basate typical terminology for BPMD?

Copy link
Author

Choose a reason for hiding this comment

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

They are not. I needed a name for the acid & base channel.
I went off diluted/ concentrated channel which are called diluate/concentrate (in ED documentation). Extrapolating from this I chose acidate/basate.

If anybody has better suggestion I am happy to use it.

Copy link
Contributor

Choose a reason for hiding this comment

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

In theory they are an acid concentrate and base concentrate (increasing in concentration), where the brine is a brine diluate (deceasing in concentration) based on the donation of ions toward the objective of the system, correct?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, they do increase concentration. But I think there is a subtle difference. BPMED increase concentration via production of new acid/base (through water splitting) and not by just transporting ions across the membranes. I feel like that deserved a distinction in nomenclature. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd go with either acid and base or acidic and basic.

Copy link
Author

@johnson12742 johnson12742 Oct 10, 2024

Choose a reason for hiding this comment

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

@adam-a-a That's reasonable. I have changed the names to Acidic & Basic

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.

Here are some initial comments until I finish my pass

@@ -6,6 +6,7 @@ Unit Models

anaerobic_digester
aeration_tank
Biploar_electrodialysis_0D
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Biploar_electrodialysis_0D
bipolar_electrodialysis_0D

@@ -0,0 +1,340 @@
Bipolar Electrodialysis (0D)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is great! While we're here, you should also add an rst file to describe costing, e.g.,: https://watertap.readthedocs.io/en/latest/technical_reference/costing/detailed_unit_model_costing.html

Copy link
Author

Choose a reason for hiding this comment

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

That's a good idea. However, I made some changes to the unit model costing and would like some feedback (before committing it to the docs).
While doing analysis I realised that a big component (negative cost/value addition) of the bipolar costing might be best done on the flowsheet level. So, I have removed it (in the most recent version).

As it stands the BPMED costing is very similar to the ED costing. The difference being the variable name for number of membranes. (cell_pair_num vs cell_num). Is there a way to inherit the ED costing?

Also, just for future reference I plan to add the bpmed + ED unit model (a configuration that is commonly used and something that I have been using for analysis). This would require minor alteration to the costing. It would be nice to know the capabilities/limits of the inheritance. Since that would inform whether I need a costing just for bipolar membrane.
I am happy to elaborate more on this issue.

@@ -0,0 +1,340 @@
Bipolar Electrodialysis (0D)
============================
Copy link
Contributor

@adam-a-a adam-a-a Oct 10, 2024

Choose a reason for hiding this comment

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

Looks like you updated the terminology to acidic and basic channels. One other place where this should be updated is in the png diagram.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for pointing that out. Somehow I missed that.

Comment on lines 22 to 24
ion and water transport across the membrane along with water splitting. Modelled transfer mechanisms include
electrical migration, diffusion of ions, osmosis, electroosmosis, and water splitting. The following are the key
assumptions made:
Copy link
Contributor

Choose a reason for hiding this comment

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

This sounds great. A question that comes to mind is -- what notable mechanisms does the model not account for at this time (but technically could be based on latest literature)? I suggest creating an issue to track any such mechanisms so that we chart a path for future enhancement, whether that be by you or another developer later on.

Copy link
Author

Choose a reason for hiding this comment

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

@adam-a-a This is a good question. I actually faced the other problem. It was challenging finding the appropriate steady state equations for bipolar membrane (there are many equations/features for dynamic bipolar modelling but that's a can of worms for another day).
Even now there are some features (like the boundary layer details) that have not been added to the unit model because I can't find appropriate equations.


* The **acidic** and **basic** side channels have identical geometry.
* For each channel, component fluxes are uniform in the bulk solutions (the 0-dimensional assumption) and are set as the average of inlet and outlet of each channel.
* Steady state: all variables are independent on time.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Steady state: all variables are independent on time.
* Steady state: all variables are independent of time.

Comment on lines 40 to 41
* **acidic** channel
* **basic** side channel
Copy link
Contributor

Choose a reason for hiding this comment

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

Is one called acidic and the other called basic or basic_side? I'd remove "side" if not used in the attribute name to avoid confusion.


"Time", ":math:`t`", "[t] ([0])\ :sup:`1`"
"Phase", ":math:`p`", "['Liq']"
"Component", ":math:`j`", "['H\ :sub:`2` \O', 'Na\ :sup:`+`', 'Cl\ :sup:`-`', 'H\ :sup:`+`', 'OH\ :sup:`-`']"
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not yet clear whether this unit works with MCAS, the NaCl prop package, both, and/or other property models. Can you elaborate?

Copy link
Author

Choose a reason for hiding this comment

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

How do I check this? I took what the ED folks have been using and ran with it. So, theoretically whatever is applicable for the prop packages in the ED unit model should be applicable here (with the caveat that users should also supply H_+ and OH_- ).

Comment on lines 86 to 87
to fully solve the model. The exact degrees of freedom depend on the mode of operation. For the simplest case where no water
splitting occurs and the bipolar membrane acts like a simple electrodialysis memrbane these are:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
to fully solve the model. The exact degrees of freedom depend on the mode of operation. For the simplest case where no water
splitting occurs and the bipolar membrane acts like a simple electrodialysis memrbane these are:
to fully solve the model. The exact degrees of freedom depend on the mode of operation. For the simplest case, where no water
splitting occurs and the bipolar membrane acts like a simple electrodialysis membrane, these are:

ion_dict = {
"solute_list": ["Na_+", "Cl_-", "H_+", "OH_-"],
"mw_data": {
"H2O": 18e-3,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"H2O": 18e-3,
"H2O": 18e-3,

Note, you no longer need to supply MW for water anymore.

},
}

This model, by default, uses H\ :sub:`2`\ O as the solvent of the feed solution.
Copy link
Contributor

Choose a reason for hiding this comment

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

I probably should wait until I get to the code, but what happens when the user omits H_+ and OH_- from the solute_list? Would the unit model raise the appropriate exception? If so, and I may have missed it, you should add a note here stating that H2 and OH must be provided.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, the user needs to supply H_+ and OH_- . I have now added a note saying that it must be added.

…rrect error in water flux. Updated unit model and test (changes are minor)
…rrect error in water flux. Updated unit model and test (changes are minor) v1.1
…rrect error in water flux. Updated unit model and test (changes are minor) v1.2
…rrect error in water flux. Updated unit model and test (changes are minor) v1.2

"Component mass balance", ":math:`N_{j, in}^{acidic \: or\: basic}-N_{j, out}^{acidic\: or\: basic}+J_j^{acidic\: or\: basic} bl=0`", ":math:`j \in \left['H_2 O', '{Na^+} ', '{Cl^-} '\right]`"
"mass transfer flux, basic, solute", ":math:`J_j^{C} = -t_j^{bpem}\frac{\xi i_{lim}}{ z_j F}`", ":math:`j \in \left['{Na_+} ', '{Cl^-} '\right]`"
"mass transfer flux, acidic, Water ions", ":math:`J_j^{C} = \frac{i - i_{lim}}{ z_j F}`", ":math:`j \in \left['{H^+} '\right]`"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would annotate "Water ions" or replace them with "proton" and "hydroxide".

:math:`mol\, m^{-3}` and are taken to be constants. :math:`f(E)` is the second Wien effect driven enhanacidicent of the
dissociation rate under applied electric field. It requires as input temperature and relative permittivity (:math:`\epsilon_r`).
To close the model :math:`\lambda = E_{crit} \epsilon_0 \epsilon_r / (F \sigma)`

Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend tabulating these equations along others for easier following. It could be even better to tabulate them side by side with the catalysis-modeled scenario. I also recommend referencing them more precisely, i.e., annotating which eq is from which literature.

Copy link
Author

Choose a reason for hiding this comment

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

@lbibl This is a very good suggestion. I have rewritten the doc file to tabulate the equations (and correct some errors).


Bipolar electrodialysis, an electrochemical separation technology, has primarily been used to generate acids and bases
from waste salts produced in water purification. By providing in-situ access to valuable raw materials bipolar membranes can
significantly reduce the total cost of the operation. This cell stack is shown in Figure 1 with **basic** and **acidic** channels, that produce base and acid
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence somewhat puzzles me without specifying what is the "raw material" and what operation.

.. csv-table:: **Table 2.** List of Degree of Freedom (DOF)
:header: "Description", "Symbol", "Variable Name", "Index", "Units", "DOF Number \ :sup:`1`"

"Temperature, inlet_acidic", ":math:`T^acidic`", "temperature", "None", ":math:`K`", 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Please check the rst formatting in the printout throughout. For instance, it should probably be T^{acidic}, etc.


"mass transfer flux, acidic/basic, Water ions", ":math:`J_j^{C} = J_{diss}`", ":math:`j \in \left['{H^+, OH^-} '\right]`"
"mass transfer flux, acidic H\ :sub:`2`\ O", ":math:`J_j^{C} = t_w^{bpem} \left(\frac{i}{F}\right)+\left(L^{bpem} \right)\left(p_{osm}^CEM-p_{osm}^AEM \right)\left(\frac{\rho_w}{M_w}\right) - J_{diss}`", ":math:`j \in \left['H_2 O'\right]`"
"mass transfer flux, basic, H\ :sub:`2`\ O", ":math:`J_j^{C} = -t_w^{bpem} \left(\frac{i}{F}\right)-\left(L^{bpem} \right)\left(p_{osm}^CEM-p_{osm}^AEM \right)\left(\frac{\rho_w}{M_w}\right) - J_{diss}`", ":math:`j \in \left['H_2 O'\right]`"
Copy link
Contributor

Choose a reason for hiding this comment

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

Check formatting throughout.

expt_current_density[indx], rel=rel_tol
)


Copy link
Contributor

Choose a reason for hiding this comment

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

I think the purpose of such tests is to ensure the model is numerically stable over time, i.e., it reproduces key solutions over time. For such a purpose, you may wanna set a constant and small tol to assert the equality to a value that is close within acceptable errors by your estimate to the value you are citing from literatures.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is advisable for all value assertions throughout.

"Electrode areal resistance", ":math:`r_{el}`", "electrodes_resistance", "[t]", ":math:`\Omega m^2`", 1
"Cell number", ":math:`n`", "cell_num", "None", "dimensionless", 1
"Current utilization coefficient", ":math:`\xi`", "current_utilization", "None", "dimensionless", 1
"Shadow factor", ":math:`\xi`", "shadow_factor", "None", "dimensionless", 1
Copy link
Contributor

Choose a reason for hiding this comment

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

\xi is doubly used.


All equations are coded as "constraints" (Pyomo). Isothermal and isobaric conditions apply.

The model used here is derived from works by Wilhelm et al. (2002) and Ionescu, Viorel (2023).It has been validated using the bipolar membrane information available online: Fumatech, Technical Data Sheet for
Copy link
Contributor

Choose a reason for hiding this comment

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

is "Ionescu, Viorel (2023)" missing in the end references? I would make sure a reader can easily find the references for all used theories/equations.

Copy link
Contributor

Choose a reason for hiding this comment

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

On the figure and its surrounding text: I suspect you are only showing a segment of a more complete picture- should there be regular aem on the left side and cem on the right side? So that'll give another two channels into which NaCl is fed? The current picture indicates the charge isn't balanced . It's likely you are not modeling a cell involving a NaCl solution feed- in either case, the current picture and the text aren't clear about what is being simulated. You may refer to Fig 2 in "X. Tongwen / Resources, ConserTation and Recycling 37 (2002) 1–22" - I suppose you are modeling of one of the two scenarios in this figure.

Copy link
Author

Choose a reason for hiding this comment

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

This is an important point/distinction. The bipolar membrane by itself cannot support production of acids and bases. It needs AEM and CEM in parallel. I have now made it explicitly clear in the documentation that these features are not in this unit model.
I do have this unit mode (that has BPEM + ED). It needs to be cleaned up before I push it (and all the results I have shown in meetings use the BPEM + ED unit model).

My reasoning to push a standalone bipolar membrane unit model is if someone in the future needs just the bipolar part. I am open to counter arguments.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not commenting on which basic unit should be modeled, but I am unclear about the focus of the current version due to several issues:

  1. You stated that this model allows "direct comparisons with works by Mareev et al. (2020) and Melnikov (2022)," which primarily develop Nernst-Planck (N-P) models for simulating potential profiles across bipolar membranes (BPM). However, this version does not establish such a simulation, and the basis for direct comparison is unclear. The test file also references data from a third work-work-Wilhelm et al. (2002)- and the cited data is used as the model input rather than output verification.
  2. The inclusion or absence of adjacent AEM or CEM layers next to the BPM significantly impacts the model. Melnikov (2022) does not consider such adjacent layers, leading to salt ion transport characteristics (e.g., K⁺ moving in the same direction as OH⁻) that differ from scenarios with adjacent AEM/CEM layers, where counter-ion transport would be supplemented from the other side facing the BPM. Mareev et al. (2020), as well as your diagram, suggest the presence of adjacent layers, as salt counter-ions are supplemented from the opposite side of the BPM. Also, clarifying the charge identity of the BPM layers could be very helpful.
  3. Questions about mass balance for all components remain unanswered and are essential for understanding the model. If the goal is not solving N-P PDEs to establish electric field profiles but rather to account for mass transfer and balances, clear definitions of reaction terms are crucial.

Overall, I recommend clarifying the electrochemical cell scheme and explicitly defining the scope of the model. Additionally, validation against literature data in the test file would strengthen the results.

"mass transfer flux, acidic, proton", ":math:`J_j^{C} = \frac{i - i_{lim}}{F}`", ":math:`j \in \left['{H^+} '\right]`"
"mass transfer flux, acidic, hydroxide", ":math:`J_j^{C} = 0`", ":math:`j \in \left['{OH^-} '\right]`"
"mass transfer flux, basic, proton", ":math:`J_j^{C} = 0`", ":math:`j \in \left['{H^+} '\right]`"
"mass transfer flux, basic, hydroxide", ":math:`J_j^{C} = \frac{i - i_{lim}}{F}`", ":math:`j \in \left['{OH^-} '\right]`"
Copy link
Contributor

@lbibl lbibl Nov 14, 2024

Choose a reason for hiding this comment

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

If there is a flux of H+ and OH- , why are they not reflected in the mass balance eq? I also think you should show a reaction term, a critical feature of an electrolysis cell, in the overall mass balance . The namely "mass transfer" terms for H+ and OH- are essentially reaction terms by your equations, although their mass was transferred...

"Salt concentration, acidic side ", ":math:`C_{acidic}`", "salt_conc_acidic", "[bpem]",":math:`mol m^{-3}`"
"Membrane Fixed charge ", ":math:`\sigma`", "membrane_fixed_charge", "[bpem]",":math:`mol m^{-3}`"
"Dissociation rate constant, zero electric field ", ":math:`k_2(0)`", "kd_zero", "[bpem]",":math:`s^{-1}`"
"Recombination rate constant ", ":math:`k_r`", "k_r", "[bpem]",":math:`L^1 mol^{-1} s^{-1}`"
Copy link
Contributor

Choose a reason for hiding this comment

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

In the code it's called kr instead of k_r

"Depletion length", ":math:`\lambda = E_{crit} \epsilon_0 \epsilon_r / (F \sigma)`", "``limiting_potential_method_bpem =LimitingpotentialMethod.Empirical``"
"Water splitting rate at electric field :math:`E` ", ":math:`R_{H^+/OH^-} (E) = [k_2(0)f(E)C_{H_2O}-k_r C_{H^+}C_{OH^-} ]`", "``limiting_potential_method_bpem =LimitingpotentialMethod.InitialValue``"
"Critical electric field", ":math:`R_{H^+/OH^-}(E = E_{crit})F/\lambda= 0.1 i_{lim}`", "``limiting_potential_method_bpem =LimitingpotentialMethod.Empirical``"

Copy link
Contributor

Choose a reason for hiding this comment

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

I am seeing that all constraints in Table 4 can be set as expressions as they would all fall in the null space in the ultimate matrix for the linear solver - however, nothing problematic has been observed so far that would absolutely prevent them from being set as constraints.

@@ -0,0 +1,2343 @@
#################################################################################
Copy link
Contributor

Choose a reason for hiding this comment

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

I would change the filename to snake case to be consistent with all other units. Additionally, maybe we should use electrodialysis_bipolar_0D just so all of the electrodialysis models will exist next to each other in alphabetical file organization, just a thought.

@declare_process_block_class("Bipolar_Electrodialysis_0D")
class BipolarElectrodialysis0DData(InitializationMixin, UnitModelBlockData):
"""
0D Bipolar and Electrodialysis Model
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
0D Bipolar and Electrodialysis Model
0D Bipolar Electrodialysis Model

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this was a typo and not intended?

CONFIG.declare(
"limiting_potential_method_bpem",
ConfigValue(
default=LimitingpotentialMethod.InitialValue,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
default=LimitingpotentialMethod.InitialValue,
default=LimitingPotentialMethod.InitialValue,

Would need to correct case throughout


.. csv-table::
:header: "Configuration Options", "Description"

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear looking at this standalone config option whether InitialValue option pulls from limiting_current_density_bpem_data or limiting_potential_data.

units=pyunits.kg * pyunits.second**-1,
doc="Base prodcued",
)
self.velocity_basic = Var(
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a pending PR that changes this way of having of _basic and _acidic variable to instead being indexed by a flow_channel set, maybe worth considering after an initial push.

initialize=55 * 1e3,
bounds=(0, 1e6),
units=pyunits.kg * pyunits.second**-1,
doc="Acid prodcued",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
doc="Acid prodcued",
doc="Acid produced",

correct throughout

self.elec_field_non_dim = Var(
self.flowsheet().time,
initialize=1,
# bounds=(0, 1e32),
Copy link
Contributor

Choose a reason for hiding this comment

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

No bounds or domain here is bad practice for optimization even if it is a parameter. Also, is non_dim indicating dimensionless?

units=pyunits.second**-1,
doc="Dissociation rate constant at no electric field",
)
self.salt_conc_aem = Var(
Copy link
Contributor

Choose a reason for hiding this comment

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

Having some variables denoted as _bpem and some as either _cem or _aem is a little challenging to track how the bpem is parameterized.

)

# Fluxes Vars for constructing mass transfer terms
self.generation_cem_flux_in = Var(
Copy link
Contributor

Choose a reason for hiding this comment

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

These appear to be additive components of the flux equations, are the intermediate variables just for tractability?

"must be both used or unused at the same time. "
)

# Build Constraints
Copy link
Contributor

Choose a reason for hiding this comment

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

Note for myself, only got to here on review but wanted to publish initial comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:Normal Normal Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants