numerical-mooc / assignment-bank

Contribute alternative assignments for Numerical Methods with Python
MIT License
14 stars 75 forks source link

Li Lin's final project #19

Open LiLinGitHub opened 9 years ago

labarba commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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!