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

SCFPotential: optimization for axisymmetric potentials and added docs #281

Merged
merged 20 commits into from Aug 5, 2016

Conversation

SeaifanAladdin
Copy link
Contributor

Updated tests, added documentations. Added axi symmetric optimization.

…imeError and RuntimeWarning for invalid expansion coefficients. Sets isNonAxi depending on whether Acos and Asin corresponds to an axi symmetric matrix or not.
…w returns Acos with shape (N,L,1) and Asin=None. scf_compute_coeffs_spherical now returns Asin=None. Init throws runtimeError if Asin=None, M!=1 and M != L. Adjusted code to handle Asin=None with M=1. Fixed computeArray and computeForceArray such that it can work with a multidimensional array.
…ing through the arrays and checking that it matches the results we got
@jobovy jobovy self-assigned this Aug 4, 2016
@jobovy
Copy link
Owner

jobovy commented Aug 4, 2016

Could you merge merge-scf-twopowertri from the main galpy branch into your SCF_C branch first and then push those changes? Right now there is a conflict, which means that the tests don't run.

@coveralls
Copy link

coveralls commented Aug 4, 2016

Coverage Status

Coverage decreased (-87.3%) to 12.413% when pulling 4b63175 on SeaifanAladdin:SCF_C into 9af62b8 on jobovy:merge-scf-twopowertri.

@codecov-io
Copy link

codecov-io commented Aug 4, 2016

Current coverage is 99.63% (diff: 95.74%)

Merging #281 into merge-scf-twopowertri will decrease coverage by 0.04%

@@           merge-scf-twopowertri       #281   diff @@
=======================================================
  Files                        107         36      -71   
  Lines                      19008       4608   -14400   
  Methods                        0          0            
  Messages                       0          0            
  Branches                       0          0            
=======================================================
- Hits                       18947       4591   -14356   
+ Misses                        61         17      -44   
  Partials                       0          0            

Powered by Codecov. Last update 9af62b8...df9abe1

@jobovy
Copy link
Owner

jobovy commented Aug 4, 2016

Thanks! The build now fails for what seems to be an issue with the latest version of scipy that I reported here: scipy/scipy#6458

I think we'll have to use a work-around until this is fixed. Could you replace the line in .travis.yml that reads

conda create -q -n test-environment python=$TRAVIS_PYTHON_VERSION numpy scipy matplotlib setuptools pip cython>=0.20 nose==1.3.4

with

conda create -q -n test-environment python=$TRAVIS_PYTHON_VERSION numpy scipy=0.17.0 matplotlib setuptools pip cython>=0.20 nose==1.3.4

? This way we'll be using the older version.

@coveralls
Copy link

coveralls commented Aug 4, 2016

Coverage Status

Coverage decreased (-0.05%) to 99.631% when pulling df9abe1 on SeaifanAladdin:SCF_C into 9af62b8 on jobovy:merge-scf-twopowertri.

@jobovy jobovy merged commit e0d956d into jobovy:merge-scf-twopowertri Aug 5, 2016
@jobovy
Copy link
Owner

jobovy commented Aug 5, 2016

Thanks, this looks good. Something weird has happened to the test coverage reporting that I think is related to one of the software packages used in the test-coverage reporting being updated in a backward incompatible way 😡😡😡 I'll merge this and try to fix this.

@jobovy
Copy link
Owner

jobovy commented Aug 5, 2016

Okay, that fixed it and the merge-scf-twopowertri branch now does test coverage correctly again; you might want to merge this last change into your own branch to get this in in case we make other changes.

The test coverage is here, looks great only ten lines not covered! https://codecov.io/gh/jobovy/galpy/tree/6037e50cfc1dbbbd74053ea6433ba30f947d6ad7/galpy
As far as I can tell, the C code cannot be touched (the lines are for the evaluation of the potential in the non-axisymmetric case, which currently does not get used anywhere). There's a few lines in the SCFPotential.py file that could probably be touched, but it's not a big deal. Great job!

The documentation is also online now:

http://galpy.readthedocs.io/en/merge-scf-twopowertri/reference/potential.html

Maybe check this quickly to make sure it looks the same as what you had, but it should.

@jobovy jobovy changed the title Scf c SCFPotential: optimization for axisymmetric potentials and added dos Aug 5, 2016
@jobovy jobovy changed the title SCFPotential: optimization for axisymmetric potentials and added dos SCFPotential: optimization for axisymmetric potentials and added docs Aug 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants