Integrate findobj into photutils #44

larrybradley · 2014-05-15T22:49:03Z

This massive PR merges my findobj package into photutils (and I was able to preserve the findobj commit history).

I placed the new files (detection.py, findstars.py, and morphology.py) in a directory called detection, but the functions are exposed at the photutils top level. I placed findobj/utils/scale_img.py into photutils/utils/scale_img.py. We can move things around later as needed.

@cdeil I didn't reorganize any of the tests, but we can decide on the best approach later.

cdeil · 2014-05-16T07:20:46Z

Wow ... this is a huge contribution ... thanks!

Let's discuss in the hangout today whether we want to merge this quickly or do an extensive review here.

In any case, the minimum requirement for merging is that the travis-ci build works ... at the moment it fails because you have a top-level scipy import, but scipy is an optional dependency:
https://travis-ci.org/astropy/photutils/jobs/25287638#L136

I think at the moment there are no wheels for scikit-image on the Astropy wheelhouse, but we need them here. @astrofrog @Cadair Can you produce scikit-image wheels?

astrofrog · 2014-05-16T07:25:15Z

@cdeil - scikit-image doesn't take too long to build, so while it'd be nice to have the wheels, it shouldn't hold back the PR.

Cadair · 2014-05-16T08:34:28Z

There are now skimage wheels in my wheelhouse I think.
On 16 May 2014 08:25, "Thomas Robitaille" notifications@github.com wrote:

@cdeil https://github.com/cdeil - scikit-image doesn't take too long
to build, so while it'd be nice to have the wheels, it shouldn't hold back
the PR.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/44#issuecomment-43304255
.

cdeil · 2014-05-16T12:18:44Z

@Cadair Thanks for making the scikit-image wheels ... I'll update .travis.yml to make them available to the photutils tests in #33.

astrofrog · 2014-05-16T12:31:27Z

Note - I need to still copy these to my wheelhouse (since we use this one as the primary one for astropy)

cdeil · 2014-05-16T17:04:31Z

Locally all tests in test_findstars.py fail for me because the reference text files are not found:

E               IOError: [Errno 2] No such file or directory: u'/private/var/folders/sb/4qv5j4m90pz1rw7m70rj1b1r0000gn/T/astropy-test-aPsvvu/lib.macosx-10.9-x86_64-2.7/photutils/detection/tests/data/daofind_test_fwhm00.5_thresh05.0_sigrad00.5.txt'

https://gist.github.com/cdeil/98ef22f80b108062e369

@larrybradley I think you have to add a setup_package.py file to get those installed:
http://docs.astropy.org/en/stable/development/building.html?highlight=setup_package#customizing-setup-build-for-subpackages

You should be able to get this error locally if you don't run build_ext -i.

larrybradley · 2014-05-16T17:14:00Z

Thanks, @cdeil. 9c9985e should fix the test data files not being found.

cdeil · 2014-05-16T17:26:50Z

@larrybradley Now the tests pass for me locally.

On Python 3.3 and 3.4 I see this warning printed at the end:

sys:1: ResourceWarning: unclosed file <_io.TextIOWrapper name='/dev/null' mode='w' encoding='UTF-8'>

It doesn't make the test fail, so won't hold up merging this today, but if you can get rid of it might as well...

larrybradley · 2014-05-16T18:03:58Z

@cdeil The test data files are all opened via Table.read(). I don't see where /dev/null is being opened for writing. Is this a Table.read() issue?

Does anyone else have any suggestions as to what may be causing this?

larrybradley · 2014-05-16T19:17:02Z

The tests are still failing because of the optional scipy dependency. When will it be available for the tests?

cdeil · 2014-05-16T19:47:52Z

@larrybradley You have to mark the tests that require scipy or scikit-image as described here.
Once you do that, travis-ci will skip all the source detection tests for now, but will green light and we can merge this (because I tested locally that it works).

OK?

cdeil · 2014-05-16T19:49:00Z

docs/photutils/detection.rst

+=================================
+
+.. warning::
+    These functions require `astropy`_ version 0.3.0 (or newer).


I think this warning can be removed ... we said that we'd require Astropy 0.4 for all of photutils soon.

cdeil · 2014-05-16T19:53:22Z

@larrybradley There's a bunch or warnings in the sphinx docs build:
https://travis-ci.org/astropy/photutils/jobs/25349889#L602

You have to resolve those to get green light from travis-ci (by repeatedly running python setup.py build_sphinx locally). Do you want me to take care of those now?

larrybradley · 2014-05-16T19:54:24Z

@cdeil Sure. I'm working on skipping the tests now.

cdeil · 2014-05-16T19:55:27Z

photutils/detection/findstars.py

+import warnings
+import math
+import numpy as np
+# import astropy.nddata


Remove this uncommented line.

cdeil · 2014-05-16T20:07:51Z

I've left a few inline comments ... feel free to ignore those if you don't have time for such cleanup today. I'll be back in 30 min and can get rid of the Sphinx docs warnings then.

larrybradley · 2014-05-16T20:33:46Z

Thanks, @cdeil. I've taken care of all your inline comments. I'll leave the doc warnings to you. One thing I noticed is that using the :no-heading: automodapi directive gives warnings about "SEVERE: Title level inconsistent:".

cdeil · 2014-05-16T20:38:52Z

I have to admit I'm struggling a bit with the docs.

One thing that was missing was __all__ in your files.
Now I have a ton of new functions in the docs which I guess are more for experts and should not appear on the top-level page.

cdeil · 2014-05-16T20:39:57Z

And photutils/detection/detection.py should be renamed to photutils/detection/core.py.

astrofrog · 2014-05-17T07:51:04Z

@larrybradley - it looks like this will need to be rebased, but I suspect this is just a minor conflict in the docs TOC.

namespace. (they are lower-level and fewer top-level things means it's simpler for the end-user.)

larrybradley · 2014-05-19T20:00:24Z

OK, this is rebased now. I hope this passes and we can merge it before anything else!

cdeil · 2014-05-19T20:05:14Z

photutils/detection/__init__.py

@@ -0,0 +1,5 @@
+# Licensed under a 3-clause BSD style license - see LICENSE.rst
+"""
+This subpackage contains modules and packages for interpreting data storage


Does this docstring make sense for photutils.detection?

larrybradley · 2014-05-19T20:11:28Z

Thanks, @cdeil. I don't know where that docstring came from.

cdeil · 2014-05-19T20:54:28Z

@larrybradley Now that python setup.py test --coverage works I see that photutils/detection/morphology.py and photutils/detection/core.py don't have any test coverage.

If you have a test case (e.g. one or two simulated Gauss sources?) that could be added as photutils/detection/tests/test_morphology.py and photutils/detection/tests/test_core.py files that would be good ... alternatively could you please open a new issue that tests should be added and indicate if you have time to do it in the coming weeks or if @bsipocz should take care of that?

cdeil · 2014-05-19T20:56:06Z

@larrybradley I'll be offline soon and travis-ci is super-slow today. Feel free to merge this yourself as soon as there is green light on travis-ci.

Integrate findobj into photutils

cdeil · 2014-05-20T03:46:14Z

Great!

Everyone ... if you get a chance to try out the detection routines ... check out the docs here.

coveralls · 2014-05-20T21:07:27Z

Coverage remained the same when pulling 3ba5866 on larrybradley:findobj-master into 7567400 on astropy:master.

coveralls · 2014-05-20T21:07:28Z

Coverage remained the same when pulling 3ba5866 on larrybradley:findobj-master into 7567400 on astropy:master.

Use wheels on travis

larrybradley mentioned this pull request May 15, 2014

Integrate findobj in photutils #38

Closed

larrybradley added the feature label May 15, 2014

cdeil reviewed May 16, 2014
View reviewed changes

photutils/detection/findstars.py

import warnings

import math

import numpy as np

# import astropy.nddata

Copy link

Member

cdeil May 16, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this uncommented line.

astrofrog assigned larrybradley May 17, 2014

cdeil added the detect label May 18, 2014

cdeil added this to the 0.1 milestone May 18, 2014

cdeil mentioned this pull request May 18, 2014

Update .travis.yml to use wheels #33

Merged

larrybradley and others added 16 commits May 19, 2014 13:31

removed old findobj docs directory

48ce83b

make scipy an optional dependency

d3bfda9

fixed scipy dependency in findstars.py

aa8fe85

Added detection/tests/data path to setup.py

63bda55

Skip tests that depend on scipy

f5513ff

Minor changes suggested by @cdeil

a175b01

Removed astropy version warnings from docs

fd765a5

Use pytest bundled with astropy

8ad14f0

Use scikit-image version of rescale_intensity

80576f2

Renamed detection.py to core.py

40775ef

Add __all__ to detection modules

1548ca6

Don't import detection.morphology and utils.scale_img in the top-level

d742b8f

namespace. (they are lower-level and fewer top-level things means it's simpler for the end-user.)

Fix sphinx warnings in docstrings in findstars.py

648538b

Re-organise docs a bit and get rid of Sphinx warnings

5094d1e

Set intersphinx_mapping to include scikit-image and astropy dev

9c2f233

Fixes from findobj rebase

1d95cc5

cdeil reviewed May 19, 2014
View reviewed changes

Fixed photutils/detection/__init__.py docstring

3ba5866

larrybradley added a commit that referenced this pull request May 20, 2014

Merge pull request #44 from larrybradley/findobj-master

9769fab

Integrate findobj into photutils

larrybradley merged commit 9769fab into astropy:master May 20, 2014

larrybradley deleted the findobj-master branch May 20, 2014 01:06

cdeil mentioned this pull request May 20, 2014

Additions to docs/rtd-pip-requirements for readthedocs #58

Merged

astrofrog added a commit to astrofrog/photutils that referenced this pull request Jun 11, 2014

Merge pull request astropy#44 from mwcraig/travis-wheels

93587f4

Use wheels on travis

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate findobj into photutils #44

Integrate findobj into photutils #44

larrybradley commented May 15, 2014

cdeil commented May 16, 2014

astrofrog commented May 16, 2014

Cadair commented May 16, 2014

cdeil commented May 16, 2014

astrofrog commented May 16, 2014

cdeil commented May 16, 2014

larrybradley commented May 16, 2014

cdeil commented May 16, 2014

larrybradley commented May 16, 2014

larrybradley commented May 16, 2014

cdeil commented May 16, 2014

cdeil May 16, 2014

cdeil commented May 16, 2014

larrybradley commented May 16, 2014

cdeil May 16, 2014

cdeil commented May 16, 2014

larrybradley commented May 16, 2014

cdeil commented May 16, 2014

cdeil commented May 16, 2014

astrofrog commented May 17, 2014

larrybradley commented May 19, 2014

cdeil May 19, 2014

larrybradley commented May 19, 2014

cdeil commented May 19, 2014

cdeil commented May 19, 2014

cdeil commented May 20, 2014

coveralls commented May 20, 2014

coveralls commented May 20, 2014

Integrate findobj into photutils #44

Integrate findobj into photutils #44

Conversation

larrybradley commented May 15, 2014

cdeil commented May 16, 2014

astrofrog commented May 16, 2014

Cadair commented May 16, 2014

cdeil commented May 16, 2014

astrofrog commented May 16, 2014

cdeil commented May 16, 2014

larrybradley commented May 16, 2014

cdeil commented May 16, 2014

larrybradley commented May 16, 2014

larrybradley commented May 16, 2014

cdeil commented May 16, 2014

cdeil May 16, 2014

Choose a reason for hiding this comment

cdeil commented May 16, 2014

larrybradley commented May 16, 2014

cdeil May 16, 2014

Choose a reason for hiding this comment

cdeil commented May 16, 2014

larrybradley commented May 16, 2014

cdeil commented May 16, 2014

cdeil commented May 16, 2014

astrofrog commented May 17, 2014

larrybradley commented May 19, 2014

cdeil May 19, 2014

Choose a reason for hiding this comment

larrybradley commented May 19, 2014

cdeil commented May 19, 2014

cdeil commented May 19, 2014

cdeil commented May 20, 2014

coveralls commented May 20, 2014

coveralls commented May 20, 2014