Closed Norvell-git closed 4 years ago
Woo hoo! You're the first person to make a Pull Request! I was starting to wonder if anyone would ever contribute to the project. Thank you. Now back to your Pull Request...
In your image of equations there's a range of applicable Reynolds numbers (Reb) but in the terminal velocity column I only see Rb. Is Rb supposed to be Reynolds number Reb?
Hi, thanks getting back so fast. I hope the changes are useful.
In the terminal velocity equations we use R_b which is the bubble radius; Re_b is the Reynolds number which is only used to define the flow regime (region) and as such doesn't appear in the terminal velocity equations.
To avoid confusion it might be worth changing some of the nomenclature of radius from R_b to r_b?
Ok, I see now. If this is the same nomenclature as used in the original reference then I would not make any changes to it.
I'm happy to leave it as written then.
I'm happy with the python code so far but have you had the chance to review the docs file also?
I haven't run the code or looked at the documentation yet. I'll do that tonight.
That's sounds great. This is my first time contributing here on github so I'll gladly receive any feedback!
Do you have a link to the article that you reference in the docstring? I would like to read the article to get a better understanding of your code.
References
----------
[1] Peebles, Fred N. ; and Garber, Harold J. : Studies on the Motion of
Gas Bubbles in Liquids. Chem. Eng. Progr., vol. 49, no. 2, Feb. 1953,
pp. 88-97.
I'm currently seeing some technical difficulties in accessing my online library. The primary source is from the AiChE "Chemical engineering progress" which is not fully digitised and as such access to the original paper may be somewhat limited. As soon as I get access to my library resources again I'll try and find the original article, or failing that I'll point you towards a suitable secondary source.
Sorry to be so picky about this but I want to make sure the equations in the original article match your code (especially for such an old article).
Did the table of equations you provided above come from the original article? In that table, why is ub labeled as terminal velocity but you refer to ub in your code as bubble velocity?
I completely understand your worries, I'll sort out the source material for you. The table did not come from the original article, rather from a lecture series
In my code bubble velocity and terminal velocity have been used synonymously, where u_b is the terminal velocity of the bubble. I will change this so that it remains consistent throughout.
So I have a few actions to work on, to summarise:
Anything else you have picked up on?
I may have found someone with a copy of the article. They said they'll let me know later this week.
An issue I found with the code is that the ubrf()
function needs to be added to __init__.py
so it can be properly used with the Chemics package. Here's some background on packages in Python: https://docs.python.org/3.8/tutorial/modules.html#packages
That sounds like a good lead, I will contact the professor who delivered those slides and try gt a copy of the article.
Thanks for the notice on the that second point. I'll put it in.
Hi, I've reviewed my old fluid mechanics books and found a reference to the original source. It was found in "Fluid flow for chemical engineers" 1995 by Holland and Bragg, Chapter 7, p234.
It appears the correlations for the regions 1-4 was taken from Peebles and Garber whilst Region 5 was later added by Wallis (1969). I will update the docs accordingly.
I will gladly forward the whole chapter to your email address if you want to read further on the subject.
I have access to the "Fluid flow for chemical engineers" book. I'll look at it tonight and give you another update once I've read the chapter 7.
So I read the section in Chapter 7 related to this pull request. What I'll do is incorporate your work into the bubble_velocity
module that is currently in Chemics. The module will have two functions ubr_kunii
and ubr_holland
. The ubr_kunii
will be what is currently named ubr
and the ubr_holland
will be what's in this pull request. To do this I'll need to make even more changes to Chemics so I created a new branch in the repository called bubble-velocity
. Please update your pull request to merge into the bubble-velocity
branch instead of master
branch. Here's a short demo of how to do this:
https://github.blog/2016-08-15-change-the-base-branch-of-a-pull-request/
That sounds excellent, I'll work towards finishing that.
This looks good. Thank you for the contribution. I'll merge this request into the bubble-velocity branch and continue working on it. I will merge everything into the master branch later this month as part of v20.5 of the package. If I have any more questions I'll create an Issue and tag you in the conversation.
That's great! Thank you for your help in getting it implemented.
@Norvell-git Just want to mention that your first table of equations has a typo. The ub for Region 4 should start with 1.18 not 1.53.
Oh wow, well caught on that one. I'll make changes to it today
I already fixed it in the bubble-velocity branch.
That's great, thank you!
bubble_velocity_free
could be used to calculate velocity of bubbles rising through a fluid, where the fluid properties are given. This function can also handle slug flow bubbles which the previousbubble_velocity
function lacked.Never written in .rst before so that may require further review.
Table below highlights the basis of calculation.
If anything is missing please let me know.