Skip to content

Add Electrocoagulation 0D Model #1573

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

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

Conversation

kurbansitterley
Copy link
Contributor

@kurbansitterley kurbansitterley commented Apr 23, 2025

Fixes/Resolves:

Adds EC 0D model, costing, test file, and documentation.

Summary/Motivation:

This PR adds a 0D electrocoagulation model compatible with MCAS that builds off of the existing EC ZO model. In particular, this model adds the ability to estimate the overpotential required with Nernst and Tafel equations.

The model has been validated against results in both the Gu (2009) paper and the Dubrawski (2014) paper. These are provided as test cases in addition to a costing test case developed as part of the SETO project in collaboration with NMSU.

TODO:

  • add documentation

Changes proposed in this PR:

  • Add EC 0D model, costing, test file, and documentation.

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.

@ksbeattie ksbeattie added the Priority:Normal Normal Priority Issue or PR label Apr 24, 2025
)

@self.Constraint(doc="Overpotential calculation")
def eq_nernst_overpotential(b):
Copy link
Contributor

Choose a reason for hiding this comment

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

Just double checking how you got this. Could you reference the equation number from the source?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added some equation numbers as they appear in the references

Comment on lines +534 to +538
def conductivity(b):
return pyunits.convert(
prop_in.conc_mass_phase_comp["Liq", "TDS"] / b.tds_to_cond_conversion,
to_units=pyunits.S / pyunits.m,
)
Copy link
Contributor

@adam-a-a adam-a-a May 8, 2025

Choose a reason for hiding this comment

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

Do you think we should eventually add this (or similar) to MCAS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. If I was an experimentalist with a cond > TDS parameter that I knew worked for my system, I would want to have this option.

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.

Nice job -- most of my comments are trivial but might be important for consistency and enabling users to use more widely (or more easily when interconnecting with other units).

@adam-a-a
Copy link
Contributor

adam-a-a commented May 8, 2025

RTD check failing possibly because of missing/misplaced comma:
image

@adam-a-a adam-a-a requested review from lbibl and luohezhiming May 8, 2025 19:40
@adam-a-a
Copy link
Contributor

adam-a-a commented May 8, 2025

@luohezhiming I thought it might be good for you to review this as it might relate to your work on electroN-P

@kurbansitterley kurbansitterley marked this pull request as ready for review May 8, 2025 20:42
@kurbansitterley
Copy link
Contributor Author

@lbianchi-lbl FYI I added myself and @MuktaHardikar as CODEOWNERS for the model in this PR. Should we be doing that or let you primo devs handle that sort of thing?

@lbianchi-lbl
Copy link
Contributor

@MuktaHardikar will take a look.

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