Staeckelecc - documentation and no_median #330

jmackereth · 2018-01-04T17:57:27Z

Added documentation for new analytic orbit parameters and EccZmaxRperiRap actionAngle method.

Also added no_median option to estimateDeltaStaeckel with relevant tests.

…s (rather than median)

…haracterization, and updated the Dierickx et al. example

coveralls · 2018-01-04T19:16:16Z

Coverage increased (+2.0e-05%) to 99.782% when pulling c1bb5bd on jmackereth:staeckelecc into 549508a on jobovy:staeckelecc.

codecov-io · 2018-01-04T19:18:11Z

Codecov Report

Merging #330 into staeckelecc will increase coverage by <.01%.
The diff coverage is 100%.

@@               Coverage Diff               @@
##           staeckelecc     #330      +/-   ##
===============================================
+ Coverage        99.78%   99.78%   +<.01%     
===============================================
  Files              128      128              
  Lines            21908    21910       +2     
===============================================
+ Hits             21860    21862       +2     
  Misses              48       48

Impacted Files	Coverage Δ
galpy/actionAngle_src/actionAngleStaeckel.py	`98.92% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 549508a...7ee8a83. Read the comment docs.

jobovy

Hi Ted,

This all looks great! I made a bunch of small comments and a bigger one about the test. But I think this is very close. I'll leave the PR open for now in case we want to change something else.

jobovy · 2018-01-05T16:35:20Z

doc/source/examples/dierickx_eccentricities.py

@@ -0,0 +1,192 @@
+import os, os.path
+import pexpect


Is this actually used? Doesn't seem to be a standard library module, so users would have to install it.

think that got left in there from an earlier version i was tinkering with - can definitely be thrown out!

jobovy · 2018-01-05T16:36:43Z

tests/test_actionAngle.py

+	#and the individual ones
+	indiv = numpy.array([estimateDeltaStaeckel(MWPotential2014, o.R(ts[i]), o.z(ts[i])) for i in range(10)])
+	#check that values agree
+	assert (numpy.fabs(nomed-indiv) < 1e10).all(), 'no_median option returns different values to individual Delta estimation'


This seems like a good test, but I think it should be 1e-10 rather than 1e10!

jobovy · 2018-01-05T16:39:48Z

doc/source/orbit.rst

+for the appoximation of actions in axisymmetric potentials), without performing any orbit integration. 
+The method uses the geometry of the orbit tori to estimate the orbit parameters. After initialising 
+an ``Orbit`` instance, this is done (as in the previous section) specifying ``analytic=True`` and 
+selecting ``type='staeckel'`` (default in vX.X of galpy).


No need for the

(default in vX.X of galpy)

The documentation online is automatically the right one for this version)

jobovy · 2018-01-05T16:40:34Z

doc/source/orbit.rst

+
+Fast orbit characterization
+-----------------------
+


Somewhere early in this section need to say that this is from Mackereth & Bovy (2018, in prep) which we will later replace with the link. Don't want people to just cite Binney's paper!

jobovy · 2018-01-05T16:41:31Z

doc/source/orbit.rst

+
+>>> o.e(analytic=True, type='staeckel', pot=mp)
+
+again, where ``mp`` is the Miyamoto-Nagai potential of :ref:`Introduction:


Sorry to be nitpicking, but this mp has already been defined earlier in this page, so we can just assume that the user has done that (just like the o object itself)

jobovy · 2018-01-05T16:42:12Z

doc/source/orbit.rst

+>>> o.e(analytic=True, type='staeckel', pot=mp)
+
+again, where ``mp`` is the Miyamoto-Nagai potential of :ref:`Introduction:
+Rotation curves <rotcurves>`. This interface automatically computes the necessary Delta 


estimates rather than computes? Also 'delta' rather than 'Delta', because the actual keyword is delta

jobovy · 2018-01-05T16:44:33Z

doc/source/orbit.rst

+``phi`` for the objects. We can then optionally estimate the individual Delta parameter 
+for these phase-space points using
+
+>>> from galpy.actionAngle import estimateDeltaStaeckel


Can we move this to later so that the logical flow is

Use actionAngleStaeckel for arrays

Default of that is to use a constant delta --> show example where aAS= defined using single delta and then used

Point out that you can give EccZmaxRperiRap an array which can be computed using estimateDeltaStaeckel

jobovy · 2018-01-05T16:45:40Z

doc/source/orbit.rst

+the estimation at each point. This method is also applicable in the ``actionAngleIsochrone``, 
+``actionAngleSpherical``, and ``actionAngleAdiabatic`` modules. This method returns parameters at
+speeds as fast as 3 microseconds per object. 
+


Is it really 3 microseconds? You could put an example with timeit to explicitly show this (so people can easily test this themselves)

Can we also add an example of using the actionAngleStaeckelGrid approach and how fast that is?

jobovy · 2018-01-05T16:47:02Z

doc/source/orbit.rst

+.. _fastchar:
+
+Fast orbit characterization
+-----------------------


My sphinx build complains that the --- aren't long enough (they need to extend at least as far as the title)

jobovy · 2018-01-05T16:48:06Z

doc/source/orbit.rst

+
+We then find the following eccentricity distribution (from the numerical eccentricities)
+
+.. image:: images/dierickx-integratedehist.png


This figure is enormous on my screen, maybe we should make it smaller? I think you can add

:scale: 50 %

or similar. Same for the figures below.

…tatement from dierickx_eccentricities

coveralls · 2018-01-05T20:32:41Z

Coverage increased (+2.0e-05%) to 99.782% when pulling 9b541b9 on jmackereth:staeckelecc into 549508a on jobovy:staeckelecc.

coveralls · 2018-01-05T20:32:41Z

Coverage decreased (-13.5%) to 86.233% when pulling 9b541b9 on jmackereth:staeckelecc into 549508a on jobovy:staeckelecc.

jobovy · 2018-01-05T21:57:14Z

Thanks, this all looks good now.

I made some more small changes to the documentation, but I'm having problems pushing them to your branch. Did you click the option "Allow commits from maintainer" in the PR?

jmackereth · 2018-01-05T22:15:03Z

Thanks! Yes, i just checked and that option is on, so I'm not sure what the problem is there?

jobovy · 2018-01-05T22:38:20Z

Thanks for checking! I figured it out, for some reason git wasn't allowing me to push to your fork's staeckelecc from a branch that wasn't called staeckelecc itself (I had your changes in jmackereth-staeckelecc).

jobovy · 2018-01-05T22:47:28Z

One more thing, could you add an entry in the HISTORY.txt file about the no_median change and add yourself to the AUTHORS.txt? Please include [ci skip] in the commit messages for those changes; this causes the test suite to not be rerun.

…o staeckelecc

coveralls · 2018-01-08T10:43:19Z

Coverage increased (+2.0e-05%) to 99.782% when pulling 7ee8a83 on jmackereth:staeckelecc into 549508a on jobovy:staeckelecc.

jmackereth added 3 commits January 3, 2018 21:46

added option for estimateDeltaStaeckel to return all calculated value…

ed9688a

…s (rather than median)

Added test for estimateDeltaStaeckel no_median option

9b9217f

added documentation for orbit parameter estimation under Fast Orbit C…

c1bb5bd

…haracterization, and updated the Dierickx et al. example

jobovy requested changes Jan 5, 2018

View reviewed changes

jmackereth added 2 commits January 5, 2018 19:02

Updates to fast orbit characterization docs

d8c6669

modifications to tests for estimateDeltaStaeckel and removed import s…

9b541b9

…tatement from dierickx_eccentricities

Slight tweaks to the documentation [ci skip]

8ea0570

jobovy approved these changes Jan 5, 2018

View reviewed changes

jmackereth added 2 commits January 8, 2018 09:19

added changes to HISTORY.txt [ci skip]

d5949b5

Merge branch 'staeckelecc' of https://github.com/jmackereth/galpy int…

7ee8a83

…o staeckelecc

jobovy merged commit 653f00f into jobovy:staeckelecc Jan 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Staeckelecc - documentation and no_median #330

Staeckelecc - documentation and no_median #330

jmackereth commented Jan 4, 2018

coveralls commented Jan 4, 2018 •

edited

codecov-io commented Jan 4, 2018 •

edited

jobovy left a comment

jobovy Jan 5, 2018

jmackereth Jan 5, 2018

jobovy Jan 5, 2018

jobovy Jan 5, 2018

jobovy Jan 5, 2018

jobovy Jan 5, 2018

jobovy Jan 5, 2018

jobovy Jan 5, 2018

jobovy Jan 5, 2018

jobovy Jan 5, 2018

jobovy Jan 5, 2018

coveralls commented Jan 5, 2018 •

edited

coveralls commented Jan 5, 2018

jobovy commented Jan 5, 2018

jmackereth commented Jan 5, 2018

jobovy commented Jan 5, 2018

jobovy commented Jan 5, 2018

coveralls commented Jan 8, 2018 •

edited


		>>> o.e(analytic=True, type='staeckel', pot=mp)

		again, where ``mp`` is the Miyamoto-Nagai potential of :ref:`Introduction:


		We then find the following eccentricity distribution (from the numerical eccentricities)

		.. image:: images/dierickx-integratedehist.png

Staeckelecc - documentation and no_median #330

Staeckelecc - documentation and no_median #330

Conversation

jmackereth commented Jan 4, 2018

coveralls commented Jan 4, 2018 • edited

codecov-io commented Jan 4, 2018 • edited

Codecov Report

jobovy 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

coveralls commented Jan 5, 2018 • edited

coveralls commented Jan 5, 2018

jobovy commented Jan 5, 2018

jmackereth commented Jan 5, 2018

jobovy commented Jan 5, 2018

jobovy commented Jan 5, 2018

coveralls commented Jan 8, 2018 • edited

coveralls commented Jan 4, 2018 •

edited

codecov-io commented Jan 4, 2018 •

edited

coveralls commented Jan 5, 2018 •

edited

coveralls commented Jan 8, 2018 •

edited