Distribution function #141

antoinetavant · 2017-10-04T09:09:05Z

First contribution for me !
To start with #101, I added a 1D Maxwellian distribution function calculation.
Please tell me if you like the interface, numbre of arguments and so on.

Needs to be done :

Do the same with Kappa
implement a 2D and/or 3D version of it.
add particle generator
maybe : add symbolic formulation

pep8speaks · 2017-10-04T09:09:07Z

Hello @antoinelpp! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on October 06, 2017 at 15:48 Hours UTC

StanczakDominik

Hey, thanks for this one! I like it. There are some minor improvements I could see being useful.

I can't find a good place to put comments on the Jupyter notebook, so I'll put them here:

Check out astropy's quantity support for matplotlib
There's some minor typos, but rather than pointing them out I'll just go through the changes before merging and try to fix as many as I can :)

StanczakDominik · 2017-10-04T09:12:05Z

plasmapy/physics/tests/test_distribution.py

+    assert np.isclose(v_vect[Maxwellian_1D(v_vect,T=T_e, part='e',V_drift = V_drift ).argmax()].value,V_drift.value)
+
+    #integral of the distribution over v_vect
+    integral = (Maxwellian_1D(v_vect,T= 30000*u.K, part='e')).sum()*dv


If we used scipy.integrate here (I don't think adding scipy to requirements is an issue given we're working within the scientific Python stack), we'd also get an error estimate on the integral we could use for the next line!

I'n not so used to Scipy, I didn't knew we could do it ! I'll check it out.

StanczakDominik · 2017-10-04T09:13:25Z

plasmapy/physics/tests/test_distribution.py

+from ..distribution import (Maxwellian_1D)
+
+T_e = 30000*u.K;
+V_drift = 10*u.m/u.s;


I haven't looked at the speed of this test, but if it's not too slow (it's an integral, it shouldn't be) this would be a perfect place to test a few parameter combinations via pytest fixtures. I'd be happy to add them!

StanczakDominik · 2017-10-04T09:18:56Z

plasmapy/physics/distribution.py

+        The temperature in Kelvin
+
+    part: string, optional
+        Representation of the ion species (e.g., 'p' for protons, 'D+'


Could we rename this to particle, and say Representation of the particle species?

Ok for the argument name !

StanczakDominik · 2017-10-04T09:21:00Z

plasmapy/physics/distribution.py

+
+    else :
+        try:
+            m_s = ion_mass(ion)


This should be possible to clean up a bit once #140 goes through :)

StanczakDominik · 2017-10-04T09:22:32Z

plasmapy/physics/distribution.py

+        Representation of the ion species (e.g., 'p' for protons, 'D+'
+        for deuterium, or 'He-4 +1' for singly ionized helium-4),
+        which defaults to electrons.
+


We could probably use a latex (mathrm) representation of the formula. I can get that done for you.

Come to think of it, AFAIK SymPy can autogenerate those representations... #136 ? :D

I'm not so sure to understand this comment. I tried something tell me if this is what you think of.

My apologies, I guess I wasn't clear at all here! I added a commit to show what I mean (the actual formula for the Maxwellian distribution) within the docstring.

StanczakDominik · 2017-10-04T09:23:55Z

plasmapy/physics/distribution.py

+from ..utils import _check_quantity, check_relativistic, check_quantity
+
+
+def Maxwellian_1D(v,T,part ="e", V_drift = 0* units.m/units.s):


Seems like a nice place for @astropy.units.quantity_input :D

good point !

StanczakDominik · 2017-10-04T10:02:20Z

That's a weird test failure:

063     Example
064     -------
065     >>> from plasmapy.physics import Maxwellian_1D
066     >>> from astropy import units as u
067     >>> Maxwellian_1D(v=1*u.m/u.s, T= 30000*u.K, particle='e',V_drift=0*u.m/u.s)
Expected:
    <Quantity 5.916 m/s >
Got:
    <Quantity 5.916329687405701e-07 s / m>

so you expect something on the order of 6m/s and get 6 * 10^-7 s/m ? This seems like some bug in the formula, I'll take a look.

StanczakDominik · 2017-10-04T10:07:21Z

Okay, so I just noticed that this returns return f.to(units.s/units.m). Is this correct? It's 1/velocity, so given this is a probability distribution function it probably is correct as you can integrate it over velocities and get a 1 without units. The formula seems all right, though.

I also noticed I forgot to add k_B for each temperature in the formula docstring, sorry :(

antoinetavant · 2017-10-04T10:42:00Z

so you expect something on the order of 6m/s and get 6 * 10^-7 s/m ? This seems like some bug in the formula, I'll take a look.

No that's my mistake !

antoinetavant · 2017-10-04T12:54:20Z

Sorry all for the PR spam, but I couldn't figure out how to run the tests without pushing...
Anyone could tell me the best way to run them from my repo ?

I tried to rebase it on Upstream, but the PR is now a mess... Shouldn't I do it ? Or I maybe I did it wrong...

StanczakDominik · 2017-10-04T12:58:35Z

I wouldn't worry about rebasing for now. I think we can live with a lengthy history.

We really need to look into this: run pytest plasmapy from repository root. Could someone start an issue for this? I can't right now.

antoinetavant · 2017-10-04T13:10:03Z

Ho !
Actually pytest plasmapy works fine ! I didn't knew about it.
I'm sorry :s

StanczakDominik · 2017-10-04T13:19:40Z

No worries, glad it works for you :)

I'll try to do a final review on this but I won't be able to do that by Friday, probably. If anyone wants to step in, I'd be much obliged!

StanczakDominik

You know, I checked and this did indeed turn out to be a lot of commits. The review screen on GitHub at least turned out weird and it looks like you're kind of duplicating changes from #140. That may be a glitch. However, could you please still try rebasing onto current master and squashing some of those smaller commits after all?

I'll go through the notebook as soon as time permits :)

StanczakDominik · 2017-10-04T16:58:42Z

plasmapy/physics/__init__.py

@@ -26,3 +21,5 @@
 from .relativity import Lorentz_factor

 from .transport import Coulomb_logarithm
+
+from .distribution import (Maxwellian_1D)


Why the ( ) parentheses here?

StanczakDominik · 2017-10-04T16:59:03Z

plasmapy/physics/distribution.py

+                           Quantity)
+
+from ..constants import (m_p, m_e, c, mu0, k_B, e, eps0, pi, e)
+from ..atomic import (ion_mass, charge_state)


Once again, ( ) ?

I've made a habit of using parentheses when doing long import statements, as per PEP 328. At least to me, this makes it easier to read. It is at its most useful when import statements span multiple lines.

Oh right. I'm sorry, I forgot about this and I was checking this in a slight rush. Multilines, sure. Here, not particularly certain they're quite necessary!

namurphy · 2017-10-04T20:15:31Z

plasmapy/physics/distribution.py

+
+    # Get mass
+
+    if particle == "e":


Another way to do this is to import _is_electron from atomic and use that here:

if _is_electron(particle): m_s = m_e

This will catch when e- or electron is used to represent electrons as well.

Thank you for doing this!

Good point !
I'm not fluant with the atomic package... I should take a better look into it.

StanczakDominik · 2017-10-06T06:07:22Z

@antoinelpp I took a closer look at the branch history and moanad, because something must have gone horribly wrong there. That tree was duplicated in some weird ways. I tried everything, but in the end the only thing that made sense was to fix some of that tangled mess via rebase - you can see the result of that here, but at the same time I may have messed up some of the chronological ordering of the changes.

If you think we can let it go and let that previous messed up version rest under the sea, I made another version where it's all squashed into a single commit. Doing it that way means no worries about chronology for this PR.

antoinetavant · 2017-10-06T08:09:33Z

Hi @StanczakDominik !

I took a closer look at the branch history and moanad, because something must have gone horribly wrong there.

My bad :( I sorry about it.

we can let it go and let that previous messed up version rest under the sea,

No no no, I'd like not to mess with the branch !

I made another version where it's all squashed into a single commit.

Ok that looks way better ! I can try to squash it myself (on my repo) in order to fix the other issues. Or I can remove this PR, clean my branch and do it again... What do you think ?

Again sorry

StanczakDominik · 2017-10-06T08:24:27Z

Hey don't worry about it! Git trees are literal straight-outta-Disney magic from time to time. I think the best way to do this would be to go on your branch and sort of overwrite it with the state of my squash branch, then merge it. I'll put on my wizard robe and hat and discover a git incantation to do this. :)

…

On Oct 6, 2017 10:09, "antoinelpp" ***@***.***> wrote: Hi @StanczakDominik <https://github.com/stanczakdominik> ! I took a closer look at the branch history and moanad, because something must have gone horribly wrong there. My bad :( I sorry about it. we can let it go and let that previous messed up version rest under the sea, No no no, I'd like not to mess with the branch ! I made another version where it's all squashed into a single commit. Ok that looks way better ! I can try to squash it myself (on my repo) in order to fix the other issues. Or I can remove this PR, clean my branch and do it again... What do you think ? Again sorry — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#141 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKxDL_2sQ-R25FnyJ3Q05LBeCHMc5a1Kks5speA_gaJpZM4PtTgi> .

StanczakDominik · 2017-10-06T08:32:18Z

I have used the power of Google and divined that (sorry for placeholders, typing from phone) `git remote add Dominik CLONE-URL-GOES-HERE` `git checkout Distribution-fn` (branch name) `git reset --hard Dominik/Distribution-fn-squash` or whatever the branch name was `git push -f` should do it.

…

On Oct 6, 2017 10:24, "Dominik Stańczak" ***@***.***> wrote: Hey don't worry about it! Git trees are literal straight-outta-Disney magic from time to time. I think the best way to do this would be to go on your branch and sort of overwrite it with the state of my squash branch, then merge it. I'll put on my wizard robe and hat and discover a git incantation to do this. :) On Oct 6, 2017 10:09, "antoinelpp" ***@***.***> wrote: > Hi @StanczakDominik <https://github.com/stanczakdominik> ! > > I took a closer look at the branch history and moanad, because something > must have gone horribly wrong there. > > My bad :( I sorry about it. > > we can let it go and let that previous messed up version rest under the > sea, > > No no no, I'd like not to mess with the branch ! > > I made another version where it's all squashed into a single commit. > > Ok that looks way better ! I can try to squash it myself (on my repo) in > order to fix the other issues. Or I can remove this PR, clean my branch and > do it again... What do you think ? > > Again sorry > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#141 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AKxDL_2sQ-R25FnyJ3Q05LBeCHMc5a1Kks5speA_gaJpZM4PtTgi> > . >

created distribution.py created test_distribution.py change `part` argument to `particle` and minor docstrings Added is_electron Update formula docstring

StanczakDominik · 2017-10-06T12:43:19Z

Awesome, I'll just do a short stylistic pass on the example (TBD within two hours) and we're good to go :)

StanczakDominik · 2017-10-06T15:07:00Z

All right, take a look at this and once it's done I'd say we can merge this one :)

antoinetavant · 2017-10-06T15:49:50Z

Thanks @StanczakDominik !
I just changed the V_drift parameter in the last exemple. That's look good to me. I think we can merge it as well !

StanczakDominik · 2017-10-06T17:52:00Z

And I merged it, thanks a bunch for this one!

StanczakDominik reviewed Oct 4, 2017

View reviewed changes

namurphy reviewed Oct 4, 2017

View reviewed changes

1D maxwellian distribution

a6fbf18

created distribution.py created test_distribution.py change `part` argument to `particle` and minor docstrings Added is_electron Update formula docstring

antoinetavant force-pushed the Distrubution-fn branch from c78cfef to a6fbf18 Compare October 6, 2017 08:46

distribution example

854251b

StanczakDominik force-pushed the Distrubution-fn branch from 751232d to 854251b Compare October 6, 2017 15:06

Change parameter in exemple

7416bb7

StanczakDominik merged commit 53da3c5 into PlasmaPy:master Oct 6, 2017

lemmatum mentioned this pull request Oct 30, 2017

Added/modified 1D and 3D Maxwellian speed/velocity distributions #152

Merged

namurphy added the plasmapy.formulary Related to the plasmapy.formulary subpackage label Oct 31, 2017

lemmatum mentioned this pull request Dec 13, 2017

Functions for Maxwellian and kappa distribution functions #101

Closed

rocco8773 added this to Complete in Formulary Development Jul 9, 2020

		from ..utils import _check_quantity, check_relativistic, check_quantity


		def Maxwellian_1D(v,T,part ="e", V_drift = 0* units.m/units.s):

Distribution function #141

Distribution function #141

Conversation

antoinetavant commented Oct 4, 2017

pep8speaks commented Oct 4, 2017 • edited

Comment last updated on October 06, 2017 at 15:48 Hours UTC

StanczakDominik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StanczakDominik commented Oct 4, 2017

StanczakDominik commented Oct 4, 2017

antoinetavant commented Oct 4, 2017 • edited by StanczakDominik

antoinetavant commented Oct 4, 2017

StanczakDominik commented Oct 4, 2017

antoinetavant commented Oct 4, 2017

StanczakDominik commented Oct 4, 2017

StanczakDominik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StanczakDominik commented Oct 6, 2017

antoinetavant commented Oct 6, 2017

StanczakDominik commented Oct 6, 2017 via email

StanczakDominik commented Oct 6, 2017 via email

StanczakDominik commented Oct 6, 2017

StanczakDominik commented Oct 6, 2017

antoinetavant commented Oct 6, 2017

StanczakDominik commented Oct 6, 2017

pep8speaks commented Oct 4, 2017 •

edited

antoinetavant commented Oct 4, 2017 •

edited by StanczakDominik