DM-11041: Implement simple DIAObject to DIASource matching algorithm and flesh out DIACollection #3

morriscb · 2017-08-07T19:21:57Z

No description provided.

parejkoj

See comments.

parejkoj · 2017-08-25T19:02:57Z

python/lsst/ap/association/association.py

@@ -0,0 +1,34 @@
+# the Free Software Foundation, either version 3 of the License, or


Looks like you truncated the start of the license here?

parejkoj · 2017-08-25T19:03:51Z

python/lsst/ap/association/association.py

+# the GNU General Public License along with this program.  If not,
+# see <http://www.lsstcorp.org/LegalNotices/>.
+#
+


File docstring to say what this is supposed to do?

For my future reference this is something that should be included in every file yes?

Yes, module-level docstrings should always exist, even if they are just a sentence:

https://developer.lsst.io/docs/py_docs.html#modules

parejkoj · 2017-08-25T19:04:08Z

python/lsst/ap/association/association.py

+    pass
+
+
+class AssociationTask(pipeBase.Task):


Needs a class docstring to describe what the class is for. (is this just a stub?)

For now it is just a stub. The ticket to complete the class is DM-11747.

Put that that ticket as a TODO in the class docstring then.

parejkoj · 2017-08-25T19:22:12Z

python/lsst/ap/association/dia_collection.py

@@ -65,7 +69,7 @@ def __init__(self, dia_objects):
        self.dia_objects = dia_objects
        self._id_to_index = {}
        for idx, dia_object in enumerate(self.dia_objects):
-            self._id_to_index[dia_object.getId()] = idx
+            self._id_to_index[dia_object.get('id')] = idx


I think getId is slightly preferred, because it doesn't have to look up the "id" key, so its faster.

parejkoj · 2017-08-25T19:25:28Z

python/lsst/ap/association/dia_collection.py

@@ -106,7 +124,7 @@ def update_dia_objects(self, force=False):

        Returns
        -------
-        None
+        bool successfully updated


return type is a separate line from description:

Returns ------- int Description of anonymous integer return value.

parejkoj · 2017-08-25T20:15:36Z

tests/test_dia_collection.py

+        obj_collection.update_spatial_tree()
+        self.assertTrue(obj_collection.is_valid_tree)
+
+        self.assertEqual(len(obj_collection.get_dia_object_ids()), 2)


These two lines should just be self.assertEqual(obj_collection.get_dia_object_ids(), [1,2]), no?

parejkoj · 2017-08-25T20:16:52Z

tests/test_dia_collection.py

+        src_cat = create_test_dia_sources(5)
+        for src_idx, src in enumerate(src_cat):
+            edit_source_record(
+                src, src_idx + 4, 0.1 * src_idx, 0.1 * src_idx, -1)


Again, if you're going to split the args on new lines, do them all (and you shouldn't have to for this one)

parejkoj · 2017-08-25T20:17:43Z

tests/test_dia_collection.py

+        DIAObjectCollection.
+
+        This also tests that a DIASource that can't be associated within
+        tolerance is addpended to the DIAObjectCollection as a new


parejkoj · 2017-08-25T20:18:23Z

tests/test_dia_collection.py

+
+        self.assertFalse(np.isfinite(score_struct.scores[-1]))
+        for src_idx in range(4):
+            self.assertAlmostEqual(score_struct.scores[src_idx], 0.0,


Why shouldn't they be identically zero?

They should be. I'm being a bit cautious here as I don't want to assert that two floats be identically equal. If I don't need to check this I am happy to change it.

It's up to you: if you expect that the above items really should have a score of zero, then use exact equality. If you think they should have "very small" scores, then leave it as is.

I'll change it to identical then.

After testing this, the value is not identically zero (it's of order 1e-19). I've changed the test back to assertAlmostEqual(value1, value2, places=16). I've also added a comment stating that this value should be really close to 0.0 but not identical.

parejkoj · 2017-08-25T20:19:19Z

tests/test_dia_collection.py

+
+        for idx, obj_id in enumerate(obj_collection.get_dia_object_ids()):
+            self.assertEqual(idx, updated_indices[idx])
+            if idx != 4:


Why is idx==4 different?

I've changed this to len(dia_objects) - 1 and added a comment explaining why this index is special.

timj · 2017-08-25T20:32:29Z

There are quite a few PEP8 violations in the file. Maybe some of them were always there but given the amount of code changing can we fix them? And since we are fixing them, please add a setup.cfg to enable flake8 tests and enable test auto discovery.

timj · 2017-08-25T21:17:36Z

python/lsst/ap/association/dia_collection.py

+import lsst.afw.table as afwTable
+import lsst.pipe.base as pipeBase
+
+from .dia_object import *


Usually we don't allow wildcard imports into a non-__init__.py.

Also, I don't quite know what you mean be "setup.cfg", Tim. Is this a file I should add to the repository somewhere? If so is there an example you can link me to, please?

@morriscb your reply is to the wrong comment of @timj : I think you meant to reply to his comment just above, re: adding setup.cfg.

Unfortunately, @timj it looks like DM-10493 was closed Won't Fix, so I don't know where the documentation for configuring auto-flake8 actually lives...

It would be great if you could enable flake8 testing in this package. You do that by copying a setup.cfg from another package (until I update the dev guide). Something like this is fine:

[flake8] max-line-length = 110 ignore = E133, E226, E228, N802, N803, N806 exclude = __init__.py [tool:pytest] addopts = --flake8 flake8-ignore = E133 E226 E228 N802 N803 N806

Then you enable pytest auto discovery in tests/SConscript by adding pyList=[] to the tests() method.

Hi @timj, I tried this and I get "scons: Nothing to be done for `tests'." when using pyList=[] in the tests() method. When I remove this, scons then performs the tests. Could you take a look at the current version that I committed, please, and let me know what is missing? I Copied your text above and put it into setup.cfg in the root directory of the repository.

Are you running with an old weekly? You need current master or weekly _35.

Thanks @timj, everything works now and comes up "green".

timj · 2017-08-25T22:09:01Z

There are at least two real bugs found by flake8:

./python/lsst/ap/association/dia_collection.py:268:52: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
./python/lsst/ap/association/dia_object.py:192:15: F821 undefined name 'NotImplimentedError'

parejkoj · 2017-09-06T22:12:51Z

python/lsst/ap/association/dia_collection.py

+        if not self._is_valid_tree:
+            self.update_spatial_tree()
+
+        scores = np.ones(len(dia_source_catalog)) * np.nan


If you're just making an array of NaNs, use np.empty, since you don't need to initialize it.

I need the the NaNs to have a positive sign so that they end up at the end of a sort. I'd like to keep this then, unless there is a way to do this with np.empty

Per our conversation, NaN isn't signed in an obvious sense, and what you really want is np.inf(or the negative thereof).

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.

Finalized tests for second reviewer pass Completed responses for second reviewer pass. Added auto-linting/flake8-ing to scons scrips. Changed assertEqual in test_dia_collection.py Also fixed linting in association.py Fixed spelling error. Fixed some linting errors in test_dia_collection.py

Implimented first pass at match and score.

97068d4

parejkoj requested changes Aug 25, 2017

View reviewed changes

timj changed the title ~~tickets/DM-11041: Implement simple DIAObject to DIASource matching algorithm and flesh out DIACollection~~ DM-11041: Implement simple DIAObject to DIASource matching algorithm and flesh out DIACollection Aug 25, 2017

timj reviewed Aug 25, 2017

View reviewed changes

parejkoj reviewed Sep 6, 2017

View reviewed changes

parejkoj approved these changes Sep 7, 2017

View reviewed changes

morriscb added 4 commits September 7, 2017 15:47

Finished first unittest for dia_collection.py

5f3654b

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.

Simplified score and match methods.

56c93d3

Changed unittest to reflect new simple association.

e6102c7

Finished unittest and matching implementation.

morriscb force-pushed the tickets/DM-11041 branch from 56ffcee to 3c72d54 Compare September 7, 2017 20:49

morriscb merged commit 3c72d54 into master Sep 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-11041: Implement simple DIAObject to DIASource matching algorithm and flesh out DIACollection #3

DM-11041: Implement simple DIAObject to DIASource matching algorithm and flesh out DIACollection #3

morriscb commented Aug 7, 2017

parejkoj left a comment

parejkoj Aug 25, 2017

parejkoj Aug 25, 2017

morriscb Sep 1, 2017

parejkoj Sep 6, 2017

parejkoj Aug 25, 2017

morriscb Sep 1, 2017

parejkoj Sep 6, 2017

parejkoj Aug 25, 2017

parejkoj Aug 25, 2017

parejkoj Aug 25, 2017

parejkoj Aug 25, 2017

parejkoj Aug 25, 2017

parejkoj Aug 25, 2017

morriscb Sep 5, 2017 •

edited

parejkoj Sep 6, 2017

morriscb Sep 7, 2017

morriscb Sep 7, 2017

parejkoj Aug 25, 2017

morriscb Sep 5, 2017

timj commented Aug 25, 2017

timj Aug 25, 2017

morriscb Sep 5, 2017

parejkoj Sep 6, 2017

timj Sep 6, 2017

morriscb Sep 7, 2017

timj Sep 7, 2017

morriscb Sep 7, 2017

timj commented Aug 25, 2017

parejkoj Sep 6, 2017

morriscb Sep 6, 2017

parejkoj Sep 6, 2017 •

edited

		@@ -0,0 +1,34 @@
		# the Free Software Foundation, either version 3 of the License, or

DM-11041: Implement simple DIAObject to DIASource matching algorithm and flesh out DIACollection #3

DM-11041: Implement simple DIAObject to DIASource matching algorithm and flesh out DIACollection #3

Conversation

morriscb commented Aug 7, 2017

parejkoj 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

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 5, 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

timj commented Aug 25, 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

timj commented Aug 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parejkoj Sep 6, 2017 • edited

Choose a reason for hiding this comment

morriscb Sep 5, 2017 •

edited

parejkoj Sep 6, 2017 •

edited