-
Notifications
You must be signed in to change notification settings - Fork 21
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
Revise 'Mixing Functions' #178
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #178 +/- ##
==========================================
+ Coverage 83.09% 83.11% +0.02%
==========================================
Files 9 9
Lines 1479 1481 +2
Branches 319 320 +1
==========================================
+ Hits 1229 1231 +2
Misses 214 214
Partials 36 36 ☔ View full report in Codecov by Sentry. |
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.
Thanks very much for these corrections @Jaebeom-P ! I've just requested two small edits to clarify terminology.
There is one larger issue we need to resolve. The _actual calculation that occurs in the code of entropy_mix
currently returns the ideal mixing energy (it includes the "T").
At line 110:
return (ureg.R * blend.temperature.to("K") * (term_list[blend] - term_list[concentrate] - term_list[dilute])).to(
"J"
)
I think we have two choices. Option 1 is to edit the doc string to replace "ideal mixing entropy" with "ideal mixing energy", in which case we should keep the T and keep the units as Joules. Option 2) is to actually change the calculation so that it returns the mixing entropy. Since the function is called entropy_mix
, it's probably best to go with option 2, even though this will change the behavior (what we would call a "breaking change" in software development.
However, I suspect many users are still interested in calculating the ideal mixing energy, so maybe we can can add keyword argument to gibbs_energy_mix
that lets a user control whether or not to include activity corrections? e.g., gibbs_energy_mix(s1, s2, activity_correction=False)
would just return the mixing entropy times the temperature. If set to True
(default), you would get the current behavior.
What do you think? (Note: if implementing the change ingibbs_energy_mix
is more effort than you're prepared to invest right now, that's fine; I can go ahead and merge this PR as long as the docstring and behavior of entropy_mix
are fully consistent).
I have updated |
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.
Very nicely done @Jaebeom-P ! Thanks especially for ensuring that the documentation and the tests are up to date. I've requested one small change to reduce the redundancy in gibbs_mix
; once that's done I'll merge.
Thanks @Jaebeom-P ! Looks great. |
Summary
Major changes:
T
from mixing entropy equationJoules
toJoules per Kelvin
donnan_eql()
documentation.