ZGainsforth
commented
4 years ago Ok. Back to you.
Zack
On Apr 28, 2020, at 4:43 PM, Adel Qalieh notifications@github.com wrote:
@adelq commented on this pull request.
I left some comments and put up some fixes myself, take a look and review my changes when you have a chance
In .gitignore:
@@ -62,3 +62,7 @@ target/
JANAF cache directory
thermochem/JANAF_Cache/ + +# Vim +*.swp +tags Editor- and OS-specific .gitignore rules should be in a global gitignore
In thermochem/janaf.py:
Traceback (most recent call last):... ValueError: A value in x_new is above the interpolation range. """
TODO On these docstrings, they actually print array([89,90,18]) rather than [89,90,18]. Is this going to throw off CI?
The docstrings for the array worked fine, but the ones with exceptions were giving me trouble
In thermochem/janaf.py:
)
- data.columns = ['T', 'Cp', 'S', '[G-H(Tr)]/T', 'H-H(Tr)', 'Delta_fH',
- 'Delta_fG', 'log(Kf)']
data.columns = ['T', 'Cp', 'S', '[G-H(Tr)]/T', 'H-H(Tr)', 'Delta_fH',
'Delta_fG', 'log(Kf)']
Can just be deleted
In thermochem/janaf.py:
@@ -97,27 +108,48 @@ def init(self, rawdata_text): data['Delta_fH'] = 1000 data['Delta_fG'] = 1000
Handle NaNs for the phase transition points. This only affects Delta_fG, Delta_fH, and log(Kf)
- GoodIndices = np.where(np.isfinite(data['Delta_fH']))
- print(GoodIndices) Should use PEP8 variable naming
In thermochem/janaf.py:
TODO Deal well with crystal<->liquid transitions which have a below
and above value for Cp, S, etc.
- def str(self):
- rep = super().str()
- rep += "\n "
- rep += self.description
- rep += "\n Cp(%0.2f) = %0.3f J/mol/K"%(Tr, self.cp(Tr))
- rep += "\n S(%0.2f) = %0.3f J/mol/K"%(Tr, self.S(Tr))
- rep += "\n [G-H(%0.2f)]/%0.2f = %0.3f J/mol/K"%(Tr, Tr, self.gef(Tr))
- rep += "\n H-H(%0.2f) = %0.3f J/mol/K"%(Tr, self.hef(Tr))
- rep += "\n Delta_fH(%0.2f) = %0.0f J/mol"%(Tr, self.DeltaH(Tr))
- rep += "\n Delta_fG(%0.2f) = %0.0f J/mol"%(Tr, self.DeltaG(Tr))
- rep += "\n log(Kf((%0.2f)) = %0.3f"%(Tr, self.logKf(Tr))
- return rep This is so helpful
In thermochem/janaf.py:
if len(phase_record) > 1:
- raise ValueError("There are %d records matching this pattern." %
- len(phase_record))
The user has entered in data that does not uniquely select one record. Let's help him out by listing his options unless it is too many.
- print("There are %d records matching this pattern:" % len(phase_record))
- print(phase_record)
- print("")
- raise ValueError("Please select a unique record.") These debugging messages are fantastic
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.
adelq
I fixed some bugs where it wasn't successfully reading some phases due to formatting issues in the source JANAF files.
I also made some usability improvements: namely print(phase) doesn't just print the name of the class but also prints STP thermodynamic values and the name of the phase.
One thing I'm not sure about is the format for the docstrings. For example, one of them is:
but the actual return value from python is
np.array([ 82.201 88.4565 176.876 ])
I'm not sure if the testing module is smart enough to see through that or if I should modify the docstrings.