Closed fwitte closed 3 years ago
Thanks for reviewing the documentation in detail. It has lead to significant improvements. I'm addressing each point separately so that it is easy to follow:
I think combining mixture property estimation with the ability to model reactions may be a highlight of this package, too. Maybe consider adding it to the highlights section on the landing page.
For rather inexperienced users who want to use the git version, you may add
$ cd thermosteam
$ pip install -e .
to your installation instructions.
Include a changelog, so the user is informed about the changes since the last (major) version. Maybe there are adjustments to perform in the code.
Add some comments on how to contribute to your project including instructions on testing.
Not a required change, but a hint: Sometimes an internal link is very useful, e.g. in the Note-box of the Chemical API documentation. You can establish a link to classes/subpackages/methods/... using :py:class:/:py:mod:/:py:meth:/..., see https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html#cross-referencing-python-objects.
In the middle of 3.8, I think "vle" or something is missing between "in" and "is". "Again, the most convinient way to get and set flow rates in is through the get_flow and set_flow methods:"
In 4.3 there is a space missing between "following:" and "T": "... from the following:T (temperature ..."
End of 4.3: I am not a native English speaker, but this does sound strange to me "Here we are some examples of the possibilities:".
Adding a process overview diagram at the beginning of chapter 5 (or 6) would be very good to better comprehend the context for a new user.
In 5.3 it should be "factors" instead of "factos".
Are the first lines of chapter 6 meant to be printed bold?
At the very end of chapter 6.4. it should be ReactionItem instead of "ReationItem" and conversion instead of "converion". I too get hectic, when seeing the finish line sometimes :D.
The list at https://github.com/BioSTEAMDevelopmentGroup/thermosteam/blob/master/thermosteam/_multi_stream.py#L162-L167 is not rendered correctly, there must be an empty line before the list.
Examples header does not render correctly: https://thermosteam.readthedocs.io/en/latest/functional.html#thermosteam.functional.mixing_simple. Also, some mathematical formulas in this module do not render. I think, you need to have an empty line between the .. math:: statement and the actual formula (at least that's how I use it).
Is a word missing in the first line of https://thermosteam.readthedocs.io/en/latest/reaction/Reaction.html#thermosteam.reaction.Reaction?
The example in https://thermosteam.readthedocs.io/en/latest/_modules/thermosteam/separations.html#material_balance is not rendered. There must be an empty line after "Satisfy outlet flows:".
Do you write "i.e." with a space? In the Notes of https://thermosteam.readthedocs.io/en/latest/mixture/Mixture.html#thermosteam.mixture.Mixture.
I think you should mention to the users, that the tutorial shown is run in an ipynb and to have the same results in script based programs, some modifications are to be made:
print(pretreatment_parallel_rxn[0:2])
which prints as
ParallelReaction(["ReactionItem('Water + Glucan -> Glucose', reactant='Glucan', X=0.099, basis=mol), ReactionItem('Water + Glucan -> GlucoseOligomer', reactant='Glucan', X=0.003, basis=mol)"])
I suggest to give a hint to the user, that using
pretreatment_parallel_rxn[0:2].show()
behaves the same as the ipynb on readthedocs:
ParallelReaction (by mol):
index stoichiometry reactant X[%]
[0] Water + Glucan -> Glucose Glucan 9.90
[1] Water + Glucan -> GlucoseOligomer Glucan 0.30
import thermosteam as tmo
from matplotlib import pyplot as plt
Water = tmo.Chemical('Water')
Water.Psat.plot_vs_T([Water.Tm, Water.Tb], 'degC', 'atm', label="Water")
plt.show()
Reply: Yes, I realize many people do not use ipython consoles (like in Spyder and Jupyter Notebook). I updated all the tutorials to use .show
methods to be more explicit about this. I did not add a note as some people might miss it. I also added the matplotlib and plt.show() lines as you suggested.
Feel free to close this issue if I have addressed all your points. You're also welcome to make more comments on the documentation here too.
You're welcome, thanks for updating! I will close this for now and reopen, if I spot something more.
Actually, going through your documentation, I really think the use of ipynb in readthedocs is very nice. I am considering changing parts of the documentation in my software packages to ipynb, too :). Just need to find the time or readjust priorities :).
General
to your installation instructions.
Typos and rendering issues
Command-line execution of code instead of ipynb
I think you should mention to the users, that the tutorial shown is run in an ipynb and to have the same results in script based programs, some modifications are to be made:
which prints as
I suggest to give a hint to the user, that using
behaves the same as the ipynb on readthedocs: