radio-astro-tools / radio-beam

A simple toolkit for reading and manipulating beams from astrophysical radio spectral data cubes.
https://radio-beam.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
26 stars 21 forks source link

Optimize deconvolve #87

Closed e-koch closed 4 years ago

e-koch commented 4 years ago

Addresses #86 by adding an optimized deconvolve function that doesn't use u.Quantity and avoids extra unit conversions.

I also optimized utils.fits_in_largest by avoiding slicing Beams and forcing a Beam.__new__ call each time.

e-koch commented 4 years ago

Compared to the initial time in #86, the same timing example at the beginning is now:

681 ms ± 72.3 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

so factor of 5 improvement with these changes.

coveralls commented 4 years ago

Coverage Status

Coverage increased (+0.07%) to 85.654% when pulling b9552838ccacf4abc1f5bedcb38e0b6af1e6ee4a on e-koch:optimize_deconvolve into eced797ae260b32e3db1893092539ac99cbc2811 on radio-astro-tools:master.

coveralls commented 4 years ago

Coverage Status

Coverage decreased (-2.08%) to 83.504% when pulling 02ca0e9908cc25749338c9e61e6d4b118bea99c2 on e-koch:optimize_deconvolve into 97b2118e404c30edde74ce7d08f919ae64aa020a on radio-astro-tools:master.

keflavich commented 4 years ago

This is good to merge now, right? We still don't have CI running, though?

e-koch commented 4 years ago

No we have CI running. But, yes, good to merge after tests finish.

e-koch commented 4 years ago

Getting some infrastructure related failures for a couple cases.

e-koch commented 4 years ago

These failures may be related to the astropy 4.0.2 release 3 days ago (these errors weren't happening 4 days ago before the release). Looks like there's a build issue with 2 numpy versions being included:

Processing ./.tox/.tmp/package/1/radio-beam-0.3.3.dev47+g5c926e5.tar.gz
  Installing build dependencies: started
  Installing build dependencies: finished with status 'done'
Collecting astropy (from radio-beam==0.3.3.dev47+g5c926e5)
  Downloading https://files.pythonhosted.org/packages/12/73/4427013e2a50e4dcd17c808a3770f22ea0f27b6771decd3ad9b7f48f5753/astropy-4.0.2.tar.gz (7.8MB)
  Installing build dependencies: started
  Installing build dependencies: finished with status 'error'
  Complete output from command /home/travis/build/radio-astro-tools/radio-beam/.tox/py36-test-casa/bin/python -m pip install --ignore-installed --no-user --prefix /tmp/pip-build-env-d07tbcv5 https://files.pythonhosted.org/packages/44/a6/7fb6e8b3f4a6051e72e4e2218889351f0ee484b9ee17e995f5ccff780300/setuptools-50.3.0-py3-none-any.whl#sha256=c77b3920663a435c9450d9d971c48f5a7478fca8881b2cd2564e59f970f03536 https://files.pythonhosted.org/packages/a7/00/3df031b3ecd5444d572141321537080b40c1c25e1caa3d86cdd12e5e919c/wheel-0.35.1-py2.py3-none-any.whl#sha256=497add53525d16c173c2c1c733b8f655510e909ea78cc0e29d374243544b77a2 https://files.pythonhosted.org/packages/df/d1/4d3f8a7a920e805488a966cc6ab55c978a712240f584445d703c08b9f405/Cython-0.29.14-cp36-cp36m-manylinux1_x86_64.whl#sha256=03f6bbb380ad0acb744fb06e42996ea217e9d00016ca0ff6f2e7d60f580d0360 https://files.pythonhosted.org/packages/65/e0/eb35e762802015cab1ccee04e8a277b03f1d8e53da3ec3106882ec42558b/Jinja2-2.10.3-py2.py3-none-any.whl#sha256=74320bb91f31270f9551d46522e33af46a80c3d619f4a4bf42b3164d30b5911f https://files.pythonhosted.org/packages/90/b1/ba7e59da253c58aaf874ea790ae71d6870255a5243010d94688c41618678/numpy-1.16.6-cp36-cp36m-manylinux1_x86_64.whl#sha256=60c56922c9d759d664078fbef94132377ef1498ab27dd3d0cc7a21b346e68c06 https://files.pythonhosted.org/packages/ae/c9/69096779fd29bf3066e24124e1c88213e40bf9d2eab4786d21948a37c40b/numpy-1.17.5-cp36-cp36m-manylinux1_x86_64.whl#sha256=1739f079e2fcc985cc187aa3ce489d127a02ff12bcc5178269bb7ce5dc860e8f:
  Double requirement given: numpy==1.17.5 from https://files.pythonhosted.org/packages/ae/c9/69096779fd29bf3066e24124e1c88213e40bf9d2eab4786d21948a37c40b/numpy-1.17.5-cp36-cp36m-manylinux1_x86_64.whl#sha256=1739f079e2fcc985cc187aa3ce489d127a02ff12bcc5178269bb7ce5dc860e8f (already in numpy==1.16.6 from https://files.pythonhosted.org/packages/90/b1/ba7e59da253c58aaf874ea790ae71d6870255a5243010d94688c41618678/numpy-1.16.6-cp36-cp36m-manylinux1_x86_64.whl#sha256=60c56922c9d759d664078fbef94132377ef1498ab27dd3d0cc7a21b346e68c06, name='numpy')

@astrofrog Have you seen a similar error before? Is it the tox setup here that needs to be updated?

astrofrog commented 4 years ago

I'll investigate

astrofrog commented 4 years ago

Astropy 4.0.3 is out now, can someone at a computer restart the CI? 😃

e-koch commented 4 years ago

@astrofrog The failing linux build is passing now. The windows build is not pointing to an available python.exe. The last windows test pass has the executable at :\users\travis\build\radio-astro-tools\radio-beam\.tox\py37-test-all-dev\scripts\python.exe (https://travis-ci.org/github/radio-astro-tools/radio-beam/jobs/734438450) and the failing test is looking at c:\\tools\\miniconda3\\envs\\test\\Lib\\venv\\scripts\\nt\\python.exe (https://travis-ci.org/github/radio-astro-tools/radio-beam/jobs/735517382)

astrofrog commented 4 years ago

Yeah we are having similar issues in astropy, not sure what changed on Travis

codecov-io commented 4 years ago

Codecov Report

Merging #87 (f3d6f82) into master (1eb6956) will increase coverage by 0.07%. The diff coverage is 94.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #87      +/-   ##
==========================================
+ Coverage   85.14%   85.21%   +0.07%     
==========================================
  Files          12       12              
  Lines        1272     1292      +20     
==========================================
+ Hits         1083     1101      +18     
- Misses        189      191       +2     
Impacted Files Coverage Δ
radio_beam/utils.py 92.70% <90.00%> (-1.68%) :arrow_down:
radio_beam/beam.py 82.40% <100.00%> (+0.21%) :arrow_up:
radio_beam/commonbeam.py 70.58% <100.00%> (+1.39%) :arrow_up:
radio_beam/tests/test_beam.py 96.09% <100.00%> (ø)

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 1eb6956...f3d6f82. Read the comment docs.

e-koch commented 4 years ago

Tests passing with the CI update! @keflavich There were no material changes after your review so I'm going to merge this.