Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 entropy and enthalpy #406
Add entropy and enthalpy #406
Changes from 12 commits
8b5820b
37d6a17
f8668ee
f8de2d8
ec3d7d8
214bbfe
ffd7080
5574c3b
5e1a393
bef7d06
74b8341
eb2fa34
f312b6d
d8850fd
d4dbeb0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is enthalpy dimensionless? Should it be having energy unit?
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.
@xiki-tempula Do you want me to change the docstring? I put this since the existing documentation for free energy is:
and I'm treating enthalpy and entropy the same as free energy... it seems I should change entropy appropriately though. See the new commit.
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.
Ok, then I think this is fine. I will change this in a later PR.
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.
I see that you do add the units back.
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.
Why does entropy has unit of k? I think it shall be
u_nk.attrs["energy_unit"] + '/K'
?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.
Personally, I'm fine for this unit to be the same as other energy unit (though I know it is wrong) as it would mean that the unit conversion would work as it is (between J and cal).
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.
Free energy is in units of energy (J) and when scaled by kT, is in units of kT. Similarly entropy is in units of energy/unit temperature (J/K) and so is scaled by k, with the units of k.
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.
Reduced entropy should be unitless, entropy should be J/degree, right? Even if passed through some unitless quantities, probably the code should be clear what is going on.
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.
I think it's important to have the correct units, so I'll look into the unit conversion, I didn't consider that feature.
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.
The way alchemlyb defines the units as
u_nk.attrs['energy_unit'] = "kT"
. Adopting the same treatment for entropy to similarly have the correct scaling quantity, 'k' would be used to follow suit.If alchemlyb's definition of
u_nk.attrs['energy_unit'] = "kT"
is not the message that is best to send, that's outside of this PR. Although I like it, I think implies that it's dimensionless while also indicating what quantities should be used to restore the units (although I hope that an average user would know).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.
The
energy_unit
, allowed to be kT; kcal/mol and kJ/mol, it is just defaults to kT. I thinkk
is more like a scientific constant for most people so having it as unit is kind of bad.The unit conversion is supported between these three units, which is just a scaling factor.
So the easiest solution is to just set the entropy to have the same unit as enthalpy.
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.
To adjust the unit conversion functionality to accommodate entropy, should I add the appropriate duplicated function of
to_kT
asto_k
or add if-statements to detect the case where it's entropy? I'm leaning toward the former since running a matrix of entropy values throughto_kT()
would feel wrong. This would mean I would duplicateto_kcalmol(df, T=None)
asto_kcalmolK(df)
andto_kJmol(df, T=None)
asto_kJmolK(df)
.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.
@xiki-tempula entropy can't have the same units at kT, otherwise the existing functions would multiply it by a temperature resulting in S*T instead of S. If that's what you want to do we can and be clear in the documentation.
Although the units would still be incorrect all the time with a label of "entropy", maybe adjusting the attribute to be
delta_sT
would be best.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.
So we could have enthalpy in
kT
,kJ/mol
andkcal/mol
.The entropy should then be in
k
,kJ/mol/T
andkcal/mol/T
.In the current implementation we could have a case where enthalpy is
kJ/mol
and it is labelled correctly askJ/mol
. But we have entropy inkJ/mol/T
but is labelled ask
.