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 Acid-Base Eq.Constraints to ADM1 Documentation #1013

Merged
merged 13 commits into from
May 19, 2023

Conversation

MarcusHolly
Copy link
Contributor

Fixes/Resolves:

Part of the master issue #934

Changes proposed in this PR:

  • Add the acid-base equilibrium constraints to the ADM1 and Modified ADM1 documentation
  • Minor typo fixes in ADM1 reaction files
  • Minor corrections to ASM 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.

@MarcusHolly MarcusHolly added documentation Improvements or additions to documentation Priority:Normal Normal Priority Issue or PR labels Apr 21, 2023
@MarcusHolly MarcusHolly self-assigned this Apr 21, 2023
@MarcusHolly MarcusHolly marked this pull request as draft April 21, 2023 15:42
@codecov
Copy link

codecov bot commented Apr 21, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.82%. Comparing base (c332571) to head (61566d4).
Report is 309 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1013   +/-   ##
=======================================
  Coverage   95.82%   95.82%           
=======================================
  Files         301      301           
  Lines       28772    28772           
=======================================
  Hits        27572    27572           
  Misses       1200     1200           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MarcusHolly
Copy link
Contributor Author

Just for clarification, this PR is pretty much done. I just want to double check that I got all the equations right and that any other changes I made are accurate. So this should be marked as "ready for review" soon

@MarcusHolly MarcusHolly marked this pull request as ready for review April 24, 2023 15:57
@MarcusHolly
Copy link
Contributor Author

@agarciadiego For our implementation of the Kw, Ka,co2, and Ka,IN equations in ADM1 and modified ADM1, why do we not multiply the gas constant by 100 as done in the literature? Maybe there's a reason and I just missed it...

image

@agarciadiego
Copy link
Contributor

@MarcusHollyidaes.core.util.constantstakes care of the units gas_constant = 8.314462618 * units.joule / units.mol / units.degK

@adam-a-a adam-a-a added iedo and removed documentation Improvements or additions to documentation labels May 4, 2023
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.

Good job on this. I only think we need to recheck how certain items are categorized and ensure they are under the correct sections (e.g., S_H is an additional variable rather than a parameter)

@lbianchi-lbl lbianchi-lbl added the documentation Improvements or additions to documentation label May 4, 2023
@MarcusHolly MarcusHolly requested a review from adam-a-a May 5, 2023 12:19
@ksbeattie ksbeattie added Priority:High High Priority Issue or PR and removed Priority:Normal Normal Priority Issue or PR labels May 18, 2023
@adam-a-a adam-a-a enabled auto-merge (squash) May 19, 2023 03:13
@adam-a-a adam-a-a merged commit a57b70a into watertap-org:main May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation iedo Priority:High High Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants