DM-11039: Implement simple DIAObject #1

morriscb · 2017-07-07T22:53:29Z

No description provided.

Code needs to be tested within the stack still.

ctslater · 2017-07-24T17:37:51Z

python/lsst/ap/association/dia_object.py

+        # is currently contious and if not we make a deep copy.
+        if not self._dia_source_catalog.isContiguous():
+            tmp_dia_source_catalog = self._dia_source_catalog.copy(deep=True)
+            del self._dia_source_catalog


Is this del really necessary? It's very unpythonic.

ctslater · 2017-07-24T17:41:15Z

python/lsst/ap/association/dia_object.py

-        None
+        Return
+        ------
+        An lsst.afw data type


This is imprecise; I don't think this describes a coherent category of return types.

Changed to "A SourceRecord column value"

ctslater · 2017-07-24T18:00:50Z

python/lsst/ap/association/dia_object.py

+            if name != 'coord_ra' and name != 'coord_dec':
+                continue
+
+            mean_value = np.mean(self._dia_source_catalog[name])


This doesn't work for objects that are (e.g.) on the RA=360 line, or sources near the poles. We have a sophisticated set of operations on coordinates and angles in afw, and we should use that here (or extended it in afw as necessary) rather than ad-hoc python computations.

See previous comment. The computation also now uses afw.coord.averageCoord

ctslater · 2017-07-24T18:01:32Z

python/lsst/ap/association/dia_object.py

+            # set the the rms variables to NaN.
+            if self.n_dia_sources > 1:
+                self._dia_object_record[name + '_rms'] = afwGeom.Angle(
+                    np.std(self._dia_source_catalog[name]))


Same, should use afw-defined operations or extend afw as necessary.

There is no method for compute a covariance matrix on a coordinate. See DM-7101

ctslater · 2017-07-24T18:02:48Z

python/lsst/ap/association/dia_object.py

+                self._dia_object_record[name + '_rms'] = afwGeom.Angle(
+                    np.std(self._dia_source_catalog[name]))
+            else:
+                self._dia_object_record[name + '_rms'] = afwGeom.Angle(np.nan)


Not sure if nan is the right answer here? Numpy returns zero for this case. Either way, I don't think this is the right place to make such a choice; belongs in afw.

ctslater · 2017-07-24T18:47:17Z

python/lsst/ap/association/dia_object.py

@@ -175,24 +206,10 @@ def append_dia_source(self, input_dia_source):
        # them until we are finished adding sources.
        self._updated = False

-        # Do stuff to append this SourceRecord.
+        self._dia_source_catalog.append(input_dia_source_record)


Does this copy the source record?

Currently, this does not copy the source. This will be taken care of by the update method later which deep copies the full catalog. Would it be enough to add a comment to the docstring that the source isn't properly copied until update is run?

ctslater · 2017-07-24T18:54:00Z

python/lsst/ap/association/dia_object.py

@@ -55,12 +70,11 @@ class DIAObject(object):
        of DIASources changes and set to true when the initialize method is
        run.
    """
-
    def __init__(self, dia_source_catalog, object_source_record=None):


What's the use case for supplying the object source record? To me this is an internal data structure that shouldn't be exposed to the user.

This is for the case where you have already created a DIAObject and don't want to rerun the summary statistics on it's corresponding DIASourceCatalog. This would happen, for instance, when you persist DIAObjects and load them on a subsequent visit.

ctslater · 2017-07-24T19:08:06Z

tests/test_dia_object.py

+    def setUp(self):
+        """ Create the DIAObjct schema we will use throughout these tests.
+        """
+        self.dia_obj_schema = make_minimal_dia_object_schema()


This doesn't seem to get used?

Correct. This was left over from when before I had the create_test_dia_sources method in the test. I have removed the setUp and tearDown methds.

These functions are still here ...?

Must not have been committed. They are not removed.

ctslater · 2017-07-24T21:45:41Z

python/lsst/ap/association/dia_object.py

-    def _generate_property_methods(self):
-        """ Generate methods for accessing values stored in the
+    def get(self, name):
+        """ Return the data stored in column name within the internal


If it's internal, then the user can't know about it.

changed to stored.

Current text changed to "Retrieve a specific summary statistic value from this DIAObject."

ctslater · 2017-07-24T21:49:11Z

tests/test_dia_object.py

+        None
+        """
+
+        for name in self.dia_obj_schema.getNames():


This really doesn't need to be a loop; you already know the names of the things you want to compare to.

morriscb · 2017-07-28T00:07:37Z

Committed updated version with incorporated reviewer responses.

ctslater · 2017-07-28T22:04:44Z

python/lsst/ap/association/dia_object.py

    def __init__(self, dia_source_catalog, object_source_record=None):
        """  Create a DIAObject given an input SourceCatalog of
        DIASources.

-        Takes as input an lsst.afw.table.SourceCatalog object specifying a
+        Takes as input an lsst.afw.table.SourceCaatlog object specifying a


ctslater · 2017-07-28T22:12:38Z

python/lsst/ap/association/dia_object.py

-    Raise(NotImplementedError)
+
+    schema = afwTable.SourceTable.makeMinimalSchema()
+    # Currently we do not store 


Broken sentence

ctslater · 2017-07-28T22:13:06Z

python/lsst/ap/association/dia_object.py

+
+    # Currently the covariance of a coordinate is not defined in the
+    # stack, so we will hold off until it is implemented. This is to be
+    # addressed in DM-7101.


These comments only make sense if one knows that fields for the ra/dec uncertainty were removed. I'd drop them.

Clarified this to say "In the future we would like to use store the convariance but,... etc."

ctslater · 2017-07-28T22:14:46Z

python/lsst/ap/association/dia_object.py

+        coord_list = [src.getCoord() for src in self._dia_source_catalog]
+        ave_coord = averageCoord(coord_list)
+        return ave_coord
+        self._dia_object_record.setCoord(ave_coord)


Broken; unreachable code.

ctslater · 2017-07-28T22:16:39Z

python/lsst/ap/association/dia_object.py

+        # Right now I'm making this the same as returning the
+        # dia_source_catalog.
+
+        return self.dia_source_catalog()


This should raise NotImplementedError rather than returning something that you later plan on changing.

ctslater · 2017-07-28T22:29:25Z

tests/test_dia_object.py

+        sources = create_test_dia_sources(5)
+        dia_obj = DIAObject(sources, None)
+
+        self._compare_dia_object_values(dia_obj, 0, 2.0, 1.4142135623730951)


I don't think that actually makes it easier; say you now want to compare a mean flux, you'll still have to go to every instance of _compare_dia_object_values and change them. At least make these keyword arguments so they're not just anonymous numbers laying around.

[morriscb] I can't reply to this comment for some reason so I'll "edit it". These tests aren't going to be tests on a catalog and the idea of the function is that the values in the "source catalog" will be incremented in such as way as to make the expected values the same for each variable that is stored in the dia_object. Hence you can just compare to all the values at once and add them all into _compare_dia_object_values.

[morriscb] After testing you I have found you are correct. I have changed the input of the _compare_dia_object_values to an input dictionary of values. who's key will match up columns in the dia_object_record.

ctslater · 2017-07-28T22:33:01Z

tests/test_dia_object.py

+        self._compare_dia_object_values(dia_obj, 0, 0.0, np.nan)
+        self._compare_dia_object_values(dia_obj_dup, dia_obj.id, 0.0, np.nan)
+
+    def _compare_dia_object_values(self, dia_object, expected_id,


This function is so much nicer now. expected_std should be eliminated since it's unused.

ctslater · 2017-07-28T22:33:35Z

tests/test_dia_object.py

+    def setUp(self):
+        """ Create the DIAObjct schema we will use throughout these tests.
+        """
+        self.dia_obj_schema = make_minimal_dia_object_schema()


These functions are still here ...?

Package and classes are eups-able and scons-able

Missed some edits in last commit. Now added. Implimented second round of reviewer comments Changed to dictionary of expected values in unittest Finished second round of reviewer comments.

morriscb added 2 commits July 6, 2017 10:16

Added initial, simple implimentations for DIAObject class.

596acf9

Completed first pass on DIAObject class and unittest

6c28653

Code needs to be tested within the stack still.

ctslater reviewed Jul 24, 2017

View reviewed changes

ctslater reviewed Jul 28, 2017

View reviewed changes

morriscb added 3 commits July 31, 2017 19:47

Added initial, simple implimentations for DIAObject class.

1d12e71

Completed working first pass at implemented class.

48ba7b8

Package and classes are eups-able and scons-able

Added responses to first review comments.

2c21a0f

Missed some edits in last commit. Now added. Implimented second round of reviewer comments Changed to dictionary of expected values in unittest Finished second round of reviewer comments.

morriscb force-pushed the tickets/DM-11039 branch from 01dbeb0 to 2c21a0f Compare August 1, 2017 00:51

morriscb merged commit 2c21a0f into master Aug 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-11039: Implement simple DIAObject #1

DM-11039: Implement simple DIAObject #1

morriscb commented Jul 7, 2017

ctslater Jul 24, 2017

ctslater Jul 24, 2017

morriscb Jul 27, 2017

ctslater Jul 24, 2017

morriscb Jul 27, 2017

ctslater Jul 24, 2017

morriscb Jul 27, 2017

ctslater Jul 24, 2017

ctslater Jul 24, 2017

morriscb Jul 27, 2017

ctslater Jul 24, 2017

morriscb Jul 27, 2017

ctslater Jul 24, 2017

morriscb Jul 28, 2017

ctslater Jul 28, 2017

morriscb Jul 31, 2017

ctslater Jul 24, 2017

morriscb Jul 27, 2017

morriscb Jul 27, 2017

ctslater Jul 24, 2017

morriscb commented Jul 28, 2017

ctslater Jul 28, 2017

ctslater Jul 28, 2017

ctslater Jul 28, 2017

morriscb Jul 31, 2017

ctslater Jul 28, 2017

ctslater Jul 28, 2017

ctslater Jul 28, 2017 •

edited by morriscb

ctslater Jul 28, 2017

ctslater Jul 28, 2017

DM-11039: Implement simple DIAObject #1

DM-11039: Implement simple DIAObject #1

Conversation

morriscb commented Jul 7, 2017

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

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

morriscb commented Jul 28, 2017

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

ctslater Jul 28, 2017 • edited by morriscb

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctslater Jul 28, 2017 •

edited by morriscb