Skip to content
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

Li Lin's final project #19

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

LiLinGitHub
Copy link

No description provided.

@labarba
Copy link
Member

labarba commented Dec 17, 2014

Your code looks like C code (or worse, fortran) forcefully made to work in Python. Global variables are a terrible idea! They are dangerous and go against the idea of modularization—the reason for writing functions. If you need a variable inside a function, you should pass it as an argument. If you want to change a variable in the function, you need to return it.

Anytime you have a double (or worse, triple) loop, you should ask yourself if there is another way to write that code. This is the Python way! Nesting loops is slow, ugly and most of the time, not necessary. Things for you to think about in the future, as you progress in your Python skills. :-)

@LiLinGitHub
Copy link
Author

Thanks for your suggestion. You are right, my program runs very slow. Since it is so complicated, I got a very hard time to make it done. When I was trying to use local variables instead of global ones, I was afraid to make it more complex and make mistakes. I think I need more practice in the future.

Sent from my iPad

On Dec 17, 2014, at 13:20, Lorena A. Barba notifications@github.com wrote:

Your code looks like C code (or worse, fortran) forcefully made to work in Python. Global variables are a terrible idea! They are dangerous and go against the idea of modularization—the reason for writing functions. If you need a variable inside a function, you should pass it as an argument. If you want to change a variable in the function, you need to return it.

Anytime you have a double (or worse, triple) loop, you should ask yourself if there is another way to write that code. This is the Python way! Nesting loops is slow, ugly and most of the time, not necessary. Things for you to think about in the future, as you progress in your Python skills. :-)


Reply to this email directly or view it on GitHub.

@labarba
Copy link
Member

labarba commented Jan 5, 2015

Since you are a member of Prof. Michael Keidar's research group, I'm sure that the work you did here is useful to you in that role. For that reason, I hope you will be motivated to do some work to improve it. Here is a detailed review of your work to help you out.

The title of the notebook is long! Some of the details could be moved to an introductory paragraph, allowing for a less cumbersome title. How about:
"Plasma simulation of a simplified hall-thruster model"
or, "Particle-in-cell simulation of a simplified hall-thruster model"
Then, a short paragraph in the beginning could say that the problem results in a Poisson equation and that it will be solved in cylindrical coordinates. I would also suggest that the text on "What is a hall thruster?" should be at the top of your notebook, to motivate it.

"Such a "leapfrog" is called Particle in Cell (PIC) method." This sentence ends a confusing paragraph, overall, and it does not really make sense. I wonder if your have some concepts mixed up.

In the course, you learned about the leapfrog method in Module 1/Lesson 4
We introduced it as a way to achieve second-order accuracy in time by applying centered differences on the derivative u', where the r.h.s. of the ODE is evaluated at the midpoint between t_{n-1} and t_{n+1}.
This still applies in plasma simulations! Leapfrog is a time-integration method, where the velocity is obtained at the midpoint between two time steps.
Particle-in-cell (PIC) method refers to a different aspect of the full simulation approach: the combination of following particle trajectories (via ordinary differential equations) and solving continuous fields on a grid, with interpolations between particles and grid to couple the two.

Equation (3)—>the continuous equation that relates E with phi has not appeared previously, yet we have a discretized form here. And are you sure there should be a 2 in the denominator? If what you have on the r.h.s. is the gradient of phi, and on the numerator you have phi at points i+1 and i ... or is there a typo in the numerator?

Embedded figures: the illustrations you include in this notebook are from un-cited sources. This should be avoided: if these figures come from a copyrighted work, you could claim fair use and include them with a credit line; if they are from a licensed source, you should still include the image credit!
Are the first images from Birdsall? Or adapted from Birdsall? (if so, you still should add a credit line).
The schematic of the hall thruster is a Wikipedia image, released into the public domain. There's no obligation, but I would nevertheless add a note: "Public-domain image from Wikimedia Commons."
(I found it also on the website of David Darling without attribution.)

You are defining a constant PI with seven decimal figures. Since you already loaded NumPy, you might as well use numpy.pi with all its precision!
In any case, the code you supplied in this cell does not run. You defined the constant variable PI, and two lines below you use the variable pi, not defined. (It will work when changing that to numpy.pi)

You also define other float constants using factors like 10**(12) —there's no need for that, since Python defines floats with the symbol e indicating the power of 10, e.g., 8.854e12
See: http://www.tutorialspoint.com/python/python_numbers.htm
And you don't need to multiply by –1 to define a negative number. Just add the negative sign!

For the dependent variables, you have defined separate NumPy arrays for each component, e.g., E_x, E_r, E_theta. A better idea is to combine these into an array of arrays. You have too many variables in this code, which makes it unwieldy!

I already mentioned in my previous comment that global variables are a terrible idea. That is another issue with your code.

Did you base this code on an existing code in Keidar's group?
As submitted, the code does not run. The function def particle_movement() fails with invalid syntax in line 26. The function def remove_particle() doesn't run, either, with invalid syntax in line 8. Later on, you use the uniform and randint functions, but it doesn't look like you've loaded the random module.
You submitted broken code!

In sum, this is a valiant effort, but I think you went about it the wrong way. I'm guessing you had an existing code that you attempted to adapt. Like we have demonstrated in this course, a better way to manage the development of a new code is to start simple, build modularly, test along the way, and think about optimizations after you've proven correctness.

Typos and Style
you will we are going—>delete "you will"
an E field to effect others—>an E field to affect others
for such a complicate situation—>complicated
the only way to solve the Poisson’s equation—>the only way is to solve Poisson's equation

a little bit more complicate—>complicated
we are luckily have—>we luckily have
surfave—>surface
more complicate case—>complicated
will be present to you as an example—>will be used as an example(?)

commonly choose Xe as the fuel, since its low ionization energy—>Xe is commonly chosen as the fuel, due to its low ionization energy
molecules passing through—>molecules are passing through
there are extremely high possibility of them to be ionized—>there is a high probability of them becoming ionized

effected by the magnets—>affected by the magnets
See: http://grammarist.com/usage/affect-effect/

definations—>definition
eariler—>earlier
collistons—>collisions
bellow—>below

@LiLinGitHub
Copy link
Author

Thank you for your comments. My project is not based on any exists codes before. We did a programming homework in Prof. Keidar's class but it was a very simple program to show some spiral trajectory of charged particles in B and E fields. This project is my first time to try a complete simulation. I am very appreciate to have a chance to do that in your class.

I checked my Wakari notebook again. It seems that I damaged my code when I was editing text. If you want, I can show you a backup copy of my code which is complete and can run well but with a incomplete teaching text. It was a backup save in my half way.

I am sure I will keep working on that code for my future research of plasma in Keidar's team. I learned a lot in your class, and I will keep following the future numerical methods class online. Thanks again for your comments, it is a great help for me to improve my code.

Happy New Year!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants