Add an implementation of daophot GROUP functionality #369

mirca · 2016-06-08T20:41:27Z

Hi all,

this PR is part of a work in progress in order to implement a procedure for doing point spread function photometry in crowded fields.

A possible block-diagram of such procedure is shown here:

This PR implements the GROUP block based on Stetson (1987) that basically group stars whenever their centroid-to-centroid distance is less than crit_separation.
The FIND block is already implemented in photutils as daofind.
The NSTAR block is responsible for constructing the joint PSF model for each group recognized by GROUP and for fitting those models to the given data.
The IDENTIFY block must be able to decide which PSF models were well fitted to the data so that they can be removed from the fitting procedure.

cc @eteq @hamogu @bsipocz

bsipocz · 2016-06-08T20:56:40Z

photutils/psf/daogroup.py

+def _find_group(star, starlist, crit_separation):
+    """
+    Find those stars in `starlist` which are at a distance of
+    `crit_separation` from `star`.


These should be double back ticks as sphinx tries to linkify the singe ones and will fail with these.

ahh, never mind this wont fail the test but only because it's a private function (thus left out of the docs).

less than crite_separation ...

bsipocz · 2016-06-28T19:57:35Z

photutils/psf/groupstars.py

+__all__ = ['DAOGroup']
+
+
+class GroupStarsBase(object):


you need to add it to the public classes to __all__ to get rid of the sphinx warnings. (It is the base group for DAOGroup, thus the docs generation for the DAOGroup API tries to make a link to it, but since it's not public cannot find it).

@bsipocz Thanks Brigitta, I'll do that =)

bsipocz · 2016-06-28T23:22:00Z

docs/index.rst

@@ -52,6 +52,8 @@ User Documentation

    photutils/background.rst
    photutils/detection.rst
+    photutils/grouping.rst
+    photutils/grouping_files


you don't need to list the directory here

bsipocz · 2016-06-28T23:26:53Z

@mirca - matplotlib has a sphinx directive for plotting, and other parts of the documentation are using it. Remind me please if there is a reason for using png files for the plots rather than plotting them on the fly?

mirca · 2016-06-29T11:54:29Z

@bsipocz I realized that I could just convert an IPython notebook to rst. Would you prefer that I use the plot directive instead?

bsipocz · 2016-06-29T11:58:48Z

Yes please, it will be more consistent with the rest of the package and also avoid storing image files in the repo (though we will have to do that anyway once we set up pytest-mpl testing).

mirca · 2016-07-02T23:04:38Z

Failures seems to be unrelated.

mirca · 2016-07-07T18:51:06Z

There was an issue with the id column. It turns out that the data in the id column have to be 1, 2, ..., n rather than 0, 1, ..., n, in order to be compatible with this and this. So, should I verify if the id column is valid? I'm thinking of the case that the user will use her/his own find algorithm and it generates an id column which begins with 0...

cc @hamogu @eteq @bsipocz

hamogu · 2016-07-07T19:29:56Z

Classic problem in python: While array indices start at 0, when people talk to each other you would say "group 1, group 2, ...".
I think testing it and giving a good error message is a good idea. It's easy and its a mistake that other people will make.

bsipocz · 2016-07-09T22:54:16Z

@mirca - This needs rebasing.

mirca · 2016-07-19T04:15:59Z

@eteq @bsipocz @hamogu @larrybradley Please, consider merging this if you are happy :)

bsipocz · 2016-07-19T11:23:25Z

@mirca - As I remember @eteq said on our last telecon that he wants to review this.

bsipocz · 2016-07-26T15:18:48Z

@mirca - It seems that you've pushed the last commit to the wrong branch ;)

mirca · 2016-07-26T15:35:07Z

@bsipocz I actually forgot to commit the changes in the main file, haha, thanks! xD

eteq · 2016-08-02T09:55:00Z

photutils/psf/groupstars.py

+
+
+@six.add_metaclass(abc.ABCMeta)
+class GroupStarsBase(object):


This class and the group_stars method needs to have a docstring that lays out explicitly what the interface is (i.e., what must starlist be, and what must the method return). That way a user can implement their own grouping method if they don't like one of the options here.

Oh, and it should be very clear whether or not __call__ needs to be implemented. I see below that DAOGroup uses __call__ instead of group_stars.

My suggestion would be for this class (GroupStarsBase) to implement just this:

def __call__(self, starlist): return self.group_stars(starlist)

And then have the subclasses only implement group_stars. That way the "function"-like capability that @hamogu suggested is preserved, but subclasses have to use the group_stars method name, which is much more readable.

EDIT: (I see this is already what's in DAOGroup, but I'm just suggesting the __call__ method be moved to GroupStarsBase)

@eteq I think I addressed your points. Did I get them right?

eteq · 2016-08-02T10:02:38Z

@mirca - OK, I reviewed this and it looks great. I left a few comments above, only one of which is really significant (about making the interface in GroupStarsBase clear, and possibly a minor adjustment to where __call__ is implemented). Once you've addressed those (which should be pretty easy), this is godo to merge!

mirca · 2016-08-02T13:13:35Z

@bsipocz Do you think long URL is a fair enough reason to break pep8? ;)

bsipocz · 2016-08-02T13:17:05Z

Do you think long URL is a fair enough reason to break pep8? ;)

Sure, there are plenty of examples for that ;)
(Also the line length is one of the PEP8 that none of the packages are enforcing)

bsipocz added psf enhancement labels Jun 8, 2016

bsipocz assigned bsipocz, eteq and hamogu Jun 8, 2016

bsipocz reviewed Jun 8, 2016
View reviewed changes

mirca force-pushed the group-routine branch from ae4aca0 to c38db0e Compare June 28, 2016 18:34

bsipocz reviewed Jun 28, 2016
View reviewed changes

mirca force-pushed the group-routine branch from c376aae to 68c3530 Compare June 28, 2016 21:22

bsipocz reviewed Jun 28, 2016
View reviewed changes

mirca changed the title ~~WIP: GROUP function~~ Add an implementation of daophot GROUP functionality Jul 2, 2016

mirca added 10 commits July 19, 2016 01:13

fix docstring error

f278ee8

update doc example

c8f96f3

added changelog entry

786c0e3

make travis-ci happy

b02cb1d

fix isse with index ids

f8c5cba

test isolated sources

a3e20f2

add tests for id column

d1db9a6

use pytest.raises instance of assert_raises

e6bb930

make sphinx happy (?)

fc52cc4

add six metaclass decorator

5cea11f

mirca force-pushed the group-routine branch from 3fc153f to 5cea11f Compare July 19, 2016 04:15

make ci happy

b0e42bd

mirca added 3 commits July 23, 2016 13:36

improve doc example

5680fa6

Rename Attributes section to Parameters

df33e9a

add group algorithm using DBSCAN from sklearn

344759a

add dbscangroup class

ac1c807

eteq reviewed Aug 2, 2016
View reviewed changes

mirca added 2 commits August 2, 2016 09:46

address @eteq comments

377c85a

fix docstring

6e43bc3

bsipocz merged commit 857b38d into astropy:master Aug 2, 2016

mirca deleted the group-routine branch May 5, 2017 00:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an implementation of daophot GROUP functionality #369

Add an implementation of daophot GROUP functionality #369

mirca commented Jun 8, 2016 •

edited

Loading

bsipocz Jun 8, 2016

bsipocz Jun 8, 2016

hamogu Jun 9, 2016

bsipocz Jun 28, 2016

mirca Jun 28, 2016

bsipocz Jun 28, 2016

bsipocz commented Jun 28, 2016

mirca commented Jun 29, 2016

bsipocz commented Jun 29, 2016

mirca commented Jul 2, 2016

mirca commented Jul 7, 2016

hamogu commented Jul 7, 2016

bsipocz commented Jul 9, 2016

mirca commented Jul 19, 2016

bsipocz commented Jul 19, 2016

bsipocz commented Jul 26, 2016

mirca commented Jul 26, 2016

eteq Aug 2, 2016

eteq Aug 2, 2016 •

edited

Loading

mirca Aug 2, 2016

eteq commented Aug 2, 2016

mirca commented Aug 2, 2016

bsipocz commented Aug 2, 2016



		@six.add_metaclass(abc.ABCMeta)
		class GroupStarsBase(object):

Add an implementation of daophot GROUP functionality #369

Add an implementation of daophot GROUP functionality #369

Conversation

mirca commented Jun 8, 2016 • edited Loading

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

bsipocz commented Jun 28, 2016

mirca commented Jun 29, 2016

bsipocz commented Jun 29, 2016

mirca commented Jul 2, 2016

mirca commented Jul 7, 2016

hamogu commented Jul 7, 2016

bsipocz commented Jul 9, 2016

mirca commented Jul 19, 2016

bsipocz commented Jul 19, 2016

bsipocz commented Jul 26, 2016

mirca commented Jul 26, 2016

Choose a reason for hiding this comment

eteq Aug 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eteq commented Aug 2, 2016

mirca commented Aug 2, 2016

bsipocz commented Aug 2, 2016

mirca commented Jun 8, 2016 •

edited

Loading

eteq Aug 2, 2016 •

edited

Loading