Closed yalinli2 closed 4 years ago
Hi Yalin, thanks for the pull request and great work! I have a couple of points we could address before we can merge:
stream
instead of material
to suggest it should only be a stream (not an array)__call__
within the adiabatic_reaction
methods. This would also have the added benefit of checking the feasibility (if tmo.reaction.CHECK_FEASIBILITY is True).H_rxn
term.Could you make another pull request with the following changes?
def adiabatic_reaction(self, stream):
"""
React stream material adiabatically, accounting for the change in enthalpy
due to the heat of reaction.
Examples
--------
>>> import thermosteam as tmo
>>> import thermosteam.reaction as rxn
>>> chemicals = tmo.Chemicals(['H2O', 'H2', 'O2'])
>>> tmo.settings.set_thermo(chemicals)
>>> reaction = rxn.Reaction('2H2O -> 2H2 + O2', reactant='H2O', X=0.7)
>>> s1 = tmo.Stream('s1', H2O=1)
>>> reaction(s1)
>>> s1.show()
-> Copy and paste here whatever your console says!
"""
if not isinstance(stream, tmo.Stream):
raise ValueError(f"stream must be a Stream object, not a '{type(stream).__name__}' object")
Hnet_0 = stream.Hf + stream.H
self(stream)
stream.H = Hnet_0 - stream.Hf
Note that this example should be modified a little for the ParallelReaction and the SeriesReaction examples (just to create the ParallelReaction and a SeriesReaction object).
Unfortunately, when I reinitialized the repository a couple of weeks ago I forgot to add the tests folder. I just added them. Could you pull my changes back to your branch and then run the tests?
>>> from thermosteam.tests import test_reaction
>>> test_reaction()
I know that going through adding tests may take a while, but they have great benefit on the long run! Especially to make sure nothing breaks or gives wrong results in the future.
Ohh, just noticed that there may have been minor changes to my local file when I updated github. It would help to resolve these conflicts editing on the file (not pasting your local file). Let me know if you have trouble resolving conflicts! These can be confusing and I can help you on a call.
Thanks for the detailed instruction! So I added the examples and ran the test file. It raised tons of errors for my examples, but I don't think those are actually errors - I ran my examples in console and it worked. So I guess I wasn't following some formats.
I attached my _reaction.py (saved as .txt) and the test results here, please let me know how I should modify, thanks!
So before noting the details of the docstring, you have odd method names:
As for the docs, it looks like you ran the examples in a script, then copy pasted the script. The examples assume you run them in a console. You can try the following:
Examples
--------
Notice how the stream temperature changed after the reaction due to the heat of reaction:
>>> import thermosteam as tmo
>>> import thermosteam.reaction as rxn
>>> chemicals = tmo.Chemicals(['H2', 'O2', 'H2O'])
>>> tmo.settings.set_thermo(chemicals)
>>> reaction = rxn.Reaction('2H2 + O2 -> 2H2O', reactant='H2', X=0.7)
>>> s1 = tmo.Stream('s1', H2=10, O2=20, H2O=1000)
>>> s1.show() # BEFORE REACTION
# -> output goes here!
>>> reaction.adiabatic_reaction(s1)
>>> s1.show() # AFTER REACTION
# -> output goes here!
No need to show enthalpy since this is reflected in the temperature change. Please don't worry about detailed examples, we could go to further detail on a jupyter notebook tutorial at a later time. The example serves mostly as a test before updating the repository.
Also make sure you use ...
for a line continuation:
>>> tmo.ParallelReaction([
>>> reaction = rxn.ParallelReaction([
... # Reaction definition Reactant Conversion
... rxn.Reaction('2H2 + O2 -> 2H2O', reactant='H2', X=0.7),
... rxn.Reaction('CH4 + O2 -> CO2 + 2H2O', reactant='CH4', X=0.1)
... ])
It is also nicer to use comments instead of print (e.g. >>> # Some comment), since doctest will also want to check print statements.
Thanks for all your efforts on this Yalin, I can take care of everything else once you send me the revised file with tests passing!
Ohhhh I now understand why there are ">>>" and "..." in the docs, thanks for walking me through!
Edits are done and the attached file past test 🎉 I can send another pull request (or do I need to? I've committed and pushed to my fork) or you can directly update from you end, whichever you prefer, thanks!
Hi Yalin, it looks great and the examples are on point. Thanks again for taking the time to go through the process. This will mark a nice milestone for biosteam as the first successful pull request.
I can merge the pull request on my side, but first you'll need to update your fork. I checked your fork and it seems like the thersmoteam.reaction._reaction
module hasn't actually been updated with the adiabatic_reaction. Also, there is a ".DS_Store" file that you added in your last commit, I don't know what it is... so its best if its deleted.
Thanks!
Thanks for checking the files! I went back and forth to resolve the previous conflicts and maybe forget to push my commits to the fork at some point, it should be good now.
I opened another pull request to make it cleaner, but all the commit history is still there. The ".DS_Store" is a macOS generated file, so I updated the gitignore file to include it.
Back when I created the pull request for biorefineries, I included a gitignore file (copied a template from internet). I just realized the gitignore files for biorefineries, biosteam, and thermosteam aren't the same, so maybe you want to review and update them all, thanks!
Oh BTW for the confusing names before, I was trying to figure out which adiabatic_reaction function the test was raising error for (like Pump1, Pump2... if you remember haha)
So before noting the details of the docstring, you have odd method names:
adiabatic_reaction2 # Why is there a number? adiabatic_reaction3
@yalinli2 About the commit history, its all good. I think we all make errors with commits too.
Yeah, I remember the pump1, pump2 stuff jajaja. Yo may find it helpful to use ctrl+l (lower case L). Doctests give you line number on each failed test, so I search for the errors this way.
Thanks again!
I tried ParallelReaction.adiabatic_reaction this with the orgacids module and it seemed to be working, let me know if there's any standard test I should run, thanks!