Refactored reaction.py to add get_free_energy_of_non_charge_transfer_reaction for code reuse and circular dependency prevention #2810
Conversation
…r in the _apply_CHE_model method in reaction.py
Regression Testing Results
Detailed regression test results.Regression test aromatics:Reference: Execution time (DD:HH:MM:SS): 00:00:00:50 aromatics Passed Core Comparison ✅Original model has 15 species. aromatics Passed Edge Comparison ✅Original model has 106 species.
Observables Test Case: Aromatics Comparison
✅ All Observables varied by less than 0.500 on average between old model and new model in all conditions! aromatics Passed Observable Testing ✅Regression test liquid_oxidation:Reference: Execution time (DD:HH:MM:SS): 00:00:01:58 liquid_oxidation Passed Core Comparison ✅Original model has 37 species. liquid_oxidation Failed Edge Comparison ❌Original model has 214 species. Non-identical kinetics! ❌
kinetics:
Observables Test Case: liquid_oxidation Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! liquid_oxidation Passed Observable Testing ✅Regression test nitrogen:Reference: Execution time (DD:HH:MM:SS): 00:00:00:59 nitrogen Failed Core Comparison ❌Original model has 41 species. nitrogen Failed Edge Comparison ❌Original model has 133 species.
Observables Test Case: NC Comparison
✅ All Observables varied by less than 0.200 on average between old model and new model in all conditions! nitrogen Passed Observable Testing ✅Regression test oxidation:Reference: Execution time (DD:HH:MM:SS): 00:00:01:33 oxidation Passed Core Comparison ✅Original model has 59 species. oxidation Passed Edge Comparison ✅Original model has 230 species.
Observables Test Case: Oxidation Comparison
✅ All Observables varied by less than 0.500 on average between old model and new model in all conditions! oxidation Passed Observable Testing ✅Errors occurred during observable testing
WARNING:root:Initial mole fractions do not sum to one; normalizing.
|
|
Hi @LekiaAnonim , thanks for opening this PR. Make sure the title of the PR is concise (by default GitHub uses the full commit message of the latest commit as the title that's why it's so long). You can also make the commits more readable by having a short description in the first line and add detailed explanations after the second line when you edit the commit messages. |
LekiaAnonim
changed the title
…n.py This new method consolidates a repeated block of code previously found in _get_free_energy_of_charge_transfer_reaction and get_free_energy_of_reaction. By abstracting it, we improve code maintainability and readability. Additionally, this method is now used in get_reversible_potential to prevent a potential circular dependency: set_reference_potentials currently links to _get_free_energy_of_charge_transfer_reaction, which may in the future depend on the reference potential being set. This refactor helps decouple that logic.
Thanks @ssun30, I have an update to the PR title and commit message. |
| @@ -624,7 +624,7 @@ def _apply_CHE_model(self, T): | |||
|
|
|||
| if self.electrons != self.protons: | |||
| raise ReactionError("Number of electrons must equal number of protons! " | |||
| f"{self} has {self.electrons} protons and {self.electrons} electrons") | |||
| f"{self} has {self.protons} protons and {self.electrons} electrons") | |||
| for product in self.products: | ||
| try: | ||
| dGrxn += product.get_free_energy(T) | ||
| except Exception: | ||
| logging.error("Problem with product {!r} in reaction {!s}".format(reactant, self)) | ||
| logging.error("Problem with product {!r} in reaction {!s}".format(product, self)) |
There was a problem hiding this comment.
this typo fix looks good (but you could turn it into an f-string while you're at it)
| @@ -721,7 +714,7 @@ def get_reversible_potential(self, T): | |||
| if not self.is_charge_transfer_reaction(): | |||
| raise KineticsError("Cannot get reversible potential for non charge transfer reactions") | |||
|
|
|||
| deltaG = self._get_free_energy_of_charge_transfer_reaction(T) #J/mol | |||
| deltaG = self.get_free_energy_of_non_charge_transfer_reaction(T) #J/mol | |||
There was a problem hiding this comment.
I don't like renaming _get_free_energy_of_charge_transfer_reaction into self.get_free_energy_of_non_charge_transfer_reaction and calling it a refactoring. They sound like opposites! Was the previous wrong? what is it actually doing?
| @@ -662,25 +662,29 @@ def get_entropy_of_reaction(self, T): | |||
| for product in self.products: | |||
| dSrxn += product.get_entropy(T) | |||
| return dSrxn | |||
|
|
|||
| def get_free_energy_of_non_charge_transfer_reaction(self, T): | |||
There was a problem hiding this comment.
You'll notice the other method had cython.declare lines. This helps Cython create faster code.
Regression Testing Results
Detailed regression test results.Regression test aromatics:Reference: Execution time (DD:HH:MM:SS): 00:00:00:49 aromatics Passed Core Comparison ✅Original model has 15 species. aromatics Passed Edge Comparison ✅Original model has 106 species.
Observables Test Case: Aromatics Comparison
✅ All Observables varied by less than 0.500 on average between old model and new model in all conditions! aromatics Passed Observable Testing ✅Regression test liquid_oxidation:Reference: Execution time (DD:HH:MM:SS): 00:00:01:55 liquid_oxidation Passed Core Comparison ✅Original model has 37 species. liquid_oxidation Failed Edge Comparison ❌Original model has 214 species. Non-identical kinetics! ❌
kinetics:
Observables Test Case: liquid_oxidation Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! liquid_oxidation Passed Observable Testing ✅Regression test nitrogen:Reference: Execution time (DD:HH:MM:SS): 00:00:00:58 nitrogen Failed Core Comparison ❌Original model has 41 species. nitrogen Failed Edge Comparison ❌Original model has 133 species. Non-identical thermo! ❌
thermo: Thermo group additivity estimation: group(O2s-CdN3d) + group(N3d-OCd) + group(Cd-HN3dO) + ring(oxirene) + radical(CdJ-NdO) Non-identical kinetics! ❌
kinetics:
Observables Test Case: NC Comparison
✅ All Observables varied by less than 0.200 on average between old model and new model in all conditions! nitrogen Passed Observable Testing ✅Regression test oxidation:Reference: Execution time (DD:HH:MM:SS): 00:00:01:30 oxidation Passed Core Comparison ✅Original model has 59 species. oxidation Passed Edge Comparison ✅Original model has 230 species.
Observables Test Case: Oxidation Comparison
✅ All Observables varied by less than 0.500 on average between old model and new model in all conditions! oxidation Passed Observable Testing ✅Errors occurred during observable testing
WARNING:root:Initial mole fractions do not sum to one; normalizing.
|
In the _apply_CHE_model, in the ReactionError line,
self.electronsis used twice instead ofself.protonsandself.electrons.get_free_energy_of_non_charge_transfer_reactionThe
get_reversible_potentialmethod calculates the potential at equilibrium using the equation.dG = dGrxn - nF(V-V0) ----- (1)
at equilibrium dG = 0, and taking V = 0,
V0 can be set to V0 = dGrxn/nF ---- (2)
This means it is more preferable to calculate the
get_reversible_potentialwith reference to another methodget_free_energy_of_non_charge_transfer_reactioninstead of_get_free_energy_of_charge_transfer_reactionwhich may also be calculated based on the reference potential, if the reference potential is not zero, Eqn (1). This can introduce a circular issue because theset_reference_potentialis calculated fromget_reversible_potential.Where
get_free_energy_of_non_charge_transfer_reactionisWe can the call the
get_free_energy_of_non_charge_transfer_reactionin_get_free_energy_of_charge_transfer_reaction, andget_free_energy_of_reactionto avoid a repetition of the code block inget_free_energy_of_non_charge_transfer_reaction