-
Notifications
You must be signed in to change notification settings - Fork 7
Add phase transfer reactions #887
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds support for phase transfer reactions by introducing two new classes: HenrysLawCoefficient for calculating effective Henry's Law coefficients accounting for diprotic acid dissociation, and ReversibleRateConstant for modeling reversible reactions with temperature-dependent equilibrium constants. The PR also extends the Conditions struct with an optional pH field to support pH-dependent transfer coefficient calculations.
Key Changes
- Introduced
HenrysLawCoefficientclass that calculates effective Henry's Law coefficients with support for temperature dependence and acid dissociation equilibria (K_a1 and K_a2) - Added
ReversibleRateConstantclass for computing forward rate constants from equilibrium constants and reverse rate constants - Extended
Conditionsstruct withstd::optional<double> pHto support pH-dependent calculations
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
include/micm/process/transfer_coefficient/henrys_law_coefficient.hpp |
New header implementing effective Henry's Law coefficient calculations for diprotic acids with pH and temperature dependence |
include/micm/process/rate_constant/reversible_rate_constant.hpp |
New header implementing reversible rate constant calculations using equilibrium constant approach |
test/unit/process/transfer_coefficient/test_henrys_law_coefficient.cpp |
Comprehensive unit tests for Henry's Law coefficient including pH dependence, temperature dependence, and edge cases |
test/unit/process/rate_constant/test_reversible_rate_constant.cpp |
Comprehensive unit tests for reversible rate constant covering temperature dependence and various equilibrium scenarios |
test/unit/process/transfer_coefficient/CMakeLists.txt |
New CMake configuration to build Henry's Law coefficient tests |
test/unit/process/rate_constant/CMakeLists.txt |
Added test target for reversible rate constant |
test/unit/process/CMakeLists.txt |
Added transfer_coefficient subdirectory to build system |
include/micm/system/conditions.hpp |
Added optional pH field to support pH-dependent transfer coefficients |
include/micm/Process.hpp |
Updated to include new headers for reversible rate constant and Henry's Law coefficient |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
include/micm/process/transfer_coefficient/henrys_law_coefficient.hpp
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #887 +/- ##
==========================================
- Coverage 94.37% 94.37% -0.01%
==========================================
Files 63 65 +2
Lines 3163 3198 +35
==========================================
+ Hits 2985 3018 +33
- Misses 178 180 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
To the best of my understanding, it is correct for it to be fixed. I'm under the impression the reversible values are reached assuming the reaction is in a steady-state. But if I'm wrong, we can always change the code.
I don't know. Let's assume for now that this is appropriate. We can make a discussion for this after this PR is merged and see if we can get some time from Matt to comment on it. |
| // TODO (jiwon) - does this number make sense as the default? | ||
| // Default calculation at standard temperature (298.15 K) and neutral (7.0) | ||
| return CalculateEffective(298.15, 7.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. We really calculate the effective henry's law constant, as is noted by your variable names. The A parameter is measured at standard temperature and pressure, so 298.15 is the reference temperature. The temperature dependence of henry's law comes from the van't Hoff equation. That equation is integrated to get the final equation for effective henry's law. What we are looking at appears to be a taylor series approximation of that equation of some kind. Again, this is fine for now and we can get more information on it in a discussion and change the code later if we need to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, sounds good. I've added those comments to the discussion note.
This PR adds
ReversibleRateConstantclass andHenrysLawCoefficientclass.Those two classes are written based on the MIAM README example, and I have a few questions:
k_r, is a parameter in `ReversibleRateConstant, which represents the reverse rate constant. Is it okay that this value comes from the configuration, assuming as a fixed value?