Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-11747: Finalize AssociationTask for ap_verify #5

Merged
merged 4 commits into from Oct 3, 2017

Conversation

morriscb
Copy link
Contributor

No description provided.

Copy link
Contributor

@isullivan isullivan left a comment

Choose a reason for hiding this comment

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

Looks good. All of the tests now pass using python 3 now, except TestDIAObjectCollection.test_append_and_update and TestDIAObjectCollection.test_init which both fail because obj_collection.get_dia_object_ids() now returns a dict_keys object and not a list.

@@ -117,8 +86,8 @@ def run(self, dia_sources, exposure):
Input exposure metadata containing the bounding box for this
exposure.
"""
bbox = exposure.bbox
wcs = exposure.wcs
bbox = afwGeom.Box2D(exposure.getBBox())
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't this just be box = exposure.getBBox()? If not, please add a comment to explain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needs to be a Box2D type as other Box's do not have the method getCenter.

-1)

score_struct = dia_collection.score(
src_cat, afwGeom.Angle(1.0, units=afwGeom.arcseconds))
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't you just write 1.0*afwGeom.arcseconds?

for src_idx in range(5):
# Our scores should be extremely close to 0 but not exactly so due
# to machine noise.
self.assertFalse(np.isfinite(score_struct.scores[src_idx]))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend self.assertTrue(np.isinf(score_struct.scores[src_idx])), but this is also fine.

@morriscb
Copy link
Contributor Author

morriscb commented Oct 3, 2017

Hey @isullivan, I made the changes you requested and also wrapped the return of get_dia_object_ids in list(...). Hopefully this fixes the comparability. I'll test in python3 tomorrow (it currently passes on 2) but feel free to pull and test the code yourself and let me know.

@morriscb
Copy link
Contributor Author

morriscb commented Oct 3, 2017

Tested the code on a python3 stack and it passes. Will now merge.

Debugged test_associate_sources.

Fixed a bug in dia_collection that indexed associated DIAObjects
improperly.
Fixed a few bugs in assoc_db_sqlite and dia_collection as well.

Added a few missing docstrings.

Changed AssociationTask max_dist default back to 1.0.

Removed indexer_id from minimal_dia_source_schema method.

Forced typing for sqlite queries and afw.table calls.
Wrapped the output of DIAObjectCollection.get_object_ids as a list.
@morriscb morriscb merged commit cabfc4d into master Oct 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants