DM-11040: Implement persistence of DIAObjects and DIASources #4

morriscb · 2017-08-31T18:55:35Z

No description provided.

kfindeisen

I'd like to see a bunch of changes, ranging from better use of the DB (most important) to typos (least important). In addition to the specific comments below, I've a few general remarks.

Please think about which methods of AssociationDBSqliteTask should be public (i.e., intended for use by AssociationTask or another class) and which should be private. At the moment nearly everything is public, giving a very confusing API where it's not clear what's the correct way to do something.
I don't see a clear policy on how transactions are handled -- there's a call to _db_connection.commit in create_tables, store, and store_updated, but not load or any of the other methods (which may or may not be intended to be called externally; see my previous remark), nor do I see anything that begins a transaction. Organizing updates into coherent commits will make it much easier to parallelize source association (and may also give a performance boost).

kfindeisen · 2017-08-31T21:22:27Z

python/lsst/ap/association/assoc_db_sqlite.py

+
+afw_to_db_types = {
+    "L": "INTEGER",
+    "Angle": "REAL"


This conversion could be potentially dangerous, especially if you need to give Angles special treatment on input.

Moved this and the converse of afw SourceRecord objects into it's own class to better define the conversions.

kfindeisen · 2017-08-31T21:24:03Z

python/lsst/ap/association/assoc_db_sqlite.py

+import lsst.pex.config as pexConfig
+import lsst.pipe.base as pipeBase
+from .dia_collection import *
+from .dia_object import *


import * is discouraged. How much stuff in these two modules does AssociationDBSqliteTask need to know about?

kfindeisen · 2017-08-31T21:33:03Z

python/lsst/ap/association/assoc_db_sqlite.py

+    \anchor AssociationDBSqliteConfig_
+
+    \brief Configuration parameters for the AssociationDBSqliteTask
+    """


Please follow the dev guide when writing docstrings. Use of Doxygen (and \anchor, of all things?) is not encouraged.

Changed all docstrings to python docs.

kfindeisen · 2017-08-31T21:40:01Z

python/lsst/ap/association/assoc_db_sqlite.py

+    functions as a testing ground for the L1 database and should mimic this
+    database's eventual functionaly. This specific database implementation is
+    useful for the verification packages which may not be run with access to
+    L1 database.


Again, doesn't this sort of thing belong in RST files in doc/? Sphinx can do things like tables of contents for you, too.

kfindeisen · 2017-08-31T21:42:07Z

python/lsst/ap/association/assoc_db_sqlite.py

+    indexer = IndexerRegistry.makeField(
+        doc='Select the spatial indexer to use within the database.',
+        default='HTM'
+    )


This seems like an odd thing to make configurable. Can you explain why you'd want the indexer to be user-specified?

I'm copying a bit of the configuration from Ingest/LoadIndexedReferenceTask which does allow the indexer to be a configurable. I think there is no need to hard code a specific indexer here especially since the standard in the rest of the stack seems to make this a configurable. One thing that I may want this it is a configurable, though, is force a check on the task init that the indexer specified in the config is the same as the one used on creation of the database (and also remove all the references to HTM). I'll work on this for the next commit.

That does sound like a reasonable approach if you can make the queries indexer-agnostic.

(I guess I'm used to spatial indices having more infrastructure than just an indexer_id column, so that caught me off guard. Please disregard my earlier question about whether your DB uses spatial indexing.)

kfindeisen · 2017-09-01T19:45:41Z

tests/test_association_db_sqlite.py

+        dia_collection = DIAObjectCollection(dia_objects)
+        assoc_db = AssociationDBSqliteTask()
+        assoc_db.create_tables()
+        assoc_db.store(dia_collection, True)


These test cases all have a lot of common setup; factor it into either setUp() or a method to be called explicitly.

kfindeisen · 2017-09-01T19:46:26Z

tests/test_association_db_sqlite.py

+                dia_collection.dia_objects[obj_idx].n_dia_sources, 1)
+            for src_id, src_record in enumerate(
+                    dia_collection.dia_objects[obj_idx].dia_source_catalog):
+                self.assertEqual(src_record.getId(), src_id + obj_idx * 1)


Can you test that the records themselves are valid (e.g., value of some statistic field)?

Added a method in the unittests for doing comparisons between any given source record on a Field by Field basis.

kfindeisen · 2017-09-01T19:48:57Z

tests/test_association_db_sqlite.py

+        for obj_id in range(2):
+            for src_id in range(5):
+                assoc_db.store_dia_object_source_pair(
+                    obj_id, src_id)


Please verify that the object and source are actually associated with each other.

kfindeisen · 2017-09-01T19:49:32Z

tests/test_association_db_sqlite.py

+        assoc_db.create_tables()
+
+        assoc_db.store_dia_object(dia_objects[0])
+


Please verify that the object's info can be recovered.

kfindeisen · 2017-09-01T19:49:50Z

tests/test_association_db_sqlite.py

+
+        assoc_db.store_dia_source(dia_sources[0])
+
+        assoc_db.commit()


Please verify that the source's info can be recovered.

r-owen · 2017-09-08T18:49:52Z

@kfindeisen wrote (on a commit that has disappeared):

Wcs is being replaced by SkyWcs , which will return SpherePoint (though it currently does not). The names are a bit different: separation instead of angularSeparation.

Maybe @r-owen can comment on how the transition will look?

The transition is as follows: replace Wcs with SkyWcs on DM-10765. That is my highest priority right now, but may take another 2 weeks or so. Then replace all Coord classes with SpherePoint on DM-11162. I expect that to go fairly quickly.

morriscb · 2017-09-08T21:51:18Z

@r-owen Okay, then right now it may be best for me to keep the Coord around as I rely on the methods from another package that still uses Coord. I've made ticket DM-11868 to reflect this work.

timj · 2017-09-12T22:04:41Z

tests/test_association_db_sqlite.py

+        output_dia_collection = assoc_db.load(
+            ctr_point, afwGeom.Angle(0.2))
+
+        for obj_idx in xrange(5):


Have you run this on Python 3?

Not yet. Sorry about the xrange, Tim. Force of habit.

timj · 2017-09-12T22:05:01Z

tests/test_association_db_sqlite.py

+
+        output_dia_objects = assoc_db.get_dia_objects(indexer_ids)
+
+        for obj_idx in xrange(2):


Please fix.

timj · 2017-09-12T22:05:16Z

tests/test_association_db_sqlite.py

+
+        assoc_db.get_dia_object_records(indexer_ids)
+
+        for obj_idx in xrange(2):


Please fix.

timj · 2017-09-12T22:12:58Z

tests/test_association_db_sqlite.py

+
+class TestAssociationDBSqlite(unittest.TestCase):
+
+    def setup(self):


I don't really want empty methods here. They serve no purpose other than adding cruft, and this one in particular is no good because it should be setUp() not setup().

morriscb · 2017-09-15T18:50:23Z

Finished responses to first review pass.

kfindeisen

Looks much better. I still have some concerns about the test coverage and some SQL issues, but most of the comments are typos or style issues. (Speaking of which, do you have something that can selectively spell-check comments and docstrings?)

kfindeisen · 2017-09-19T17:05:26Z

python/lsst/ap/association/assoc_db_sqlite.py

+
+class SqliteDBConverter(object):
+    """ Class for defining conversions to and from an sqlite database and
+    afw SourceReocrd objects.


Typo: SourceRecord

kfindeisen · 2017-09-19T17:14:15Z

python/lsst/ap/association/assoc_db_sqlite.py

+        """
+        return self._schema
+
+    def make_table_from_afw_shcema(self, table_name):


Typo: schema

kfindeisen · 2017-09-19T17:29:15Z

python/lsst/ap/association/assoc_db_sqlite.py

+            if sub_schema.getField().getTypeString() == 'Angle':
+                output_source_record.set(
+                    sub_schema.getKey(),
+                    afwGeom.Angle(value * afwGeom.degrees))


Department of redundancy department: value * afwGeom.degrees is already an Angle.

kfindeisen · 2017-09-19T17:32:48Z

python/lsst/ap/association/assoc_db_sqlite.py

+
+        Parameters
+        ----------
+        soruce_record : afw.table.SourceRecord


Typo: source

kfindeisen · 2017-09-19T17:44:24Z

python/lsst/ap/association/assoc_db_sqlite.py

+    useful for the verification packages which may not be run with access to
+    L1 database.
+
+    Attriburtes


Typo: Attributes

kfindeisen · 2017-09-19T19:14:18Z

tests/test_association_db_sqlite.py

+            n_objects=1, n_sources=1, start_id=1)
+        dia_collection.append(new_dia_object[0])
+        dia_collection.update_dia_objects()
+        dia_collection.update_spatial_tree()


Consider testing what happens if you update an existing object.

kfindeisen · 2017-09-19T19:14:37Z

tests/test_association_db_sqlite.py

+        dia_collection.update_dia_objects()
+        dia_collection.update_spatial_tree()
+
+        self.assoc_db.store_updated(dia_collection, [1])


Please check that the final DB state is correct.

kfindeisen · 2017-09-19T19:18:51Z

tests/test_association_db_sqlite.py

+
+            for record_a, record_b in zip(
+                    output_dia_objects[obj_idx].dia_source_catalog,
+                    dia_collection.dia_objects[obj_idx].dia_source_catalog):


Again, can you safely assume order is preserved?

Changedthis to a similar sort on id as before.

kfindeisen · 2017-09-19T19:26:49Z

tests/test_association_db_sqlite.py

+    def _store_and_retrieve_source_record(self,
+                                          source_record,
+                                          converter):
+        """ Convinience method for round tripping a soruce


kfindeisen · 2017-09-19T19:27:50Z

python/lsst/ap/association/assoc_db_sqlite.py

+            self._db_cursor.execute(
+                "CREATE TABLE dia_objects_to_dia_sources ("
+                "src_id INTEGER, "
+                "obj_id INTEGER, "


I noticed you got rid of the primary key entirely. Why?

This was an oversight on my part when I added the FOREIGN KEY constraint. I have added the PRIMARY KEY back.

dia_collection.py: Finished first pass implimentation Added association.py Fixed bugs in dia_collection and unittest. Passes most of unittests. Need to edit score and match both within test and class. Finished unittest and matching implementation. Added initial pass at Association and database tasks. Added comments throughout. Started unittests. Added unittest skeleton for association_db_sqlite.py Changed indexing to meas_algorithms indexRegistry from sphgeom. Implemented init and create_db unittests Added PRIMARY KEYs to create_tables method. Created intial tests of the database storage. Debugged unittest for store methods.

Finished first working version of AssociationDB Added get_dia_object_records method.

Changed sqlite queries from individual to "IN". Need to account for queries that could require greater than 127 variables in the query. Added initial batch db queries. Need to limit queries to 127 variables at most. Implemented batch queries to get around this limit. Completed linting pass. Responded to some reviewer comments Stashing current commits to rebase to master. Implemented a subset of reviewer suggested changes.

Still need to confirm new API and unittests pass. Cleaned up unittests. Unittests now run to completion. Need to finialize some tests and remove rest of "magic numbers" Removed more magic numbers from unittest Cleaned up usage of _compare_source_records Finished reponses to reviewer. Committing pre-unittested responses to reviewer. Debuged unittests and finalized review respones.

kfindeisen requested changes Sep 1, 2017

View reviewed changes

morriscb force-pushed the tickets/DM-11040 branch from be734d4 to 3bcd683 Compare September 7, 2017 21:26

timj reviewed Sep 12, 2017

View reviewed changes

kfindeisen approved these changes Sep 19, 2017

View reviewed changes

morriscb added 4 commits September 21, 2017 13:10

Completed first attempt at full unittest of AssociationDB

7556caf

Finished first working version of AssociationDB Added get_dia_object_records method.

morriscb force-pushed the tickets/DM-11040 branch from b2ab9e0 to 48a7b02 Compare September 21, 2017 18:11

morriscb merged commit 48a7b02 into master Sep 21, 2017

		assoc_db.create_tables()

		assoc_db.store_dia_object(dia_objects[0])


		output_dia_objects = assoc_db.get_dia_objects(indexer_ids)

		for obj_idx in xrange(2):


		assoc_db.get_dia_object_records(indexer_ids)

		for obj_idx in xrange(2):


		class TestAssociationDBSqlite(unittest.TestCase):

		def setup(self):

DM-11040: Implement persistence of DIAObjects and DIASources #4

DM-11040: Implement persistence of DIAObjects and DIASources #4

Conversation

morriscb commented Aug 31, 2017

kfindeisen left a 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

kfindeisen Sep 7, 2017 • edited

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

r-owen commented Sep 8, 2017

morriscb commented Sep 8, 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

morriscb commented Sep 15, 2017

kfindeisen left a 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 Sep 20, 2017 • edited

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

kfindeisen Sep 7, 2017 •

edited

morriscb Sep 20, 2017 •

edited