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

Revise 'Mixing Functions' #178

Merged
merged 6 commits into from
Aug 14, 2024
Merged

Revise 'Mixing Functions' #178

merged 6 commits into from
Aug 14, 2024

Conversation

Jaebeom-P
Copy link
Contributor

Summary

Major changes:

  • feature 1: Revise 'Mixing Functions' documentation
  • fix 1: Remove T from mixing entropy equation
  • fix 2: Add explanations of activity and molar ratio for Gibbs free energy and entropy equation, respectively
  • fix 3: Revise the unit of mixing entropy from Joules to Joules per Kelvin
  • fix 4: Correct the error that prevents the formula from being displayed in the donnan_eql() documentation.

Copy link

codecov bot commented Aug 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.11%. Comparing base (b782ef6) to head (ab46f3b).
Report is 15 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

Copy link
Member

@rkingsbury rkingsbury left a 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).

src/pyEQL/functions.py Outdated Show resolved Hide resolved
src/pyEQL/functions.py Outdated Show resolved Hide resolved
@Jaebeom-P Jaebeom-P closed this Aug 13, 2024
@Jaebeom-P Jaebeom-P reopened this Aug 13, 2024
@Jaebeom-P
Copy link
Contributor Author

Jaebeom-P commented Aug 13, 2024

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 gibbs_mix (I guess you mean this, not gibbs_energy_mix) to return true energy of mixing or ideal energy of mixing corresponding to the value of activity_correction. All the documentation and instructions are referred to the documentation of Solution class.
+) Updated the test file to take account of these changes

Copy link
Member

@rkingsbury rkingsbury left a 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.

src/pyEQL/functions.py Outdated Show resolved Hide resolved
@rkingsbury rkingsbury added enhancement docs documentation labels Aug 14, 2024
@rkingsbury
Copy link
Member

Thanks @Jaebeom-P ! Looks great.

@rkingsbury rkingsbury merged commit febfcc0 into KingsburyLab:main Aug 14, 2024
13 checks passed
@rkingsbury rkingsbury changed the title Revise 'Mixing Functions' documentation Revise 'Mixing Functions' Aug 18, 2024
@rkingsbury rkingsbury added the breaking includes breaking changes label Aug 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking includes breaking changes docs documentation enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants