DM-14620: Move ap_association DIA schemas their own module #17

morriscb · 2018-06-01T21:35:48Z

No description provided.

parejkoj

See review comments. Thanks for explaining why the unittest numbers changed in the commit message.

I know a lot of these methods were copied from elsewhere, but we should clean them up.

parejkoj · 2018-06-15T20:04:50Z

python/lsst/ap/association/assoc_afw_utils.py

+           "ccdVisitSchema"]
+
+
+"""Defines afw schemas and conversions for use in ap_association tasks.


module-level docstrings should be just under the copyright:

https://developer.lsst.io/python/numpydoc.html#placement-of-module-docstrings

parejkoj · 2018-06-15T20:06:25Z

python/lsst/ap/association/assoc_afw_utils.py

+"""
+
+
+def make_minimal_dia_object_schema(filter_names=[]):


Don't default to an empty list (or any mutable). Use None instead, if it's optional, and set it to a list at the start of the method.

http://docs.python-guide.org/en/latest/writing/gotchas/#mutable-default-arguments

parejkoj · 2018-06-15T20:07:10Z

python/lsst/ap/association/assoc_afw_utils.py

+
+    # In the future we would like to store a covariance of the coordinate.
+    # This functionality is not defined in currently in the stack, so we will
+    # hold off until it is implemented. This is to be addressed in DM-7101.


Please mark this with a TODO: so it's easier to spot.

parejkoj · 2018-06-15T20:07:28Z

python/lsst/ap/association/assoc_afw_utils.py

+    # as well as their errors.
+
+    # In the future we would like to store a covariance of the coordinate.
+    # This functionality is not defined in currently in the stack, so we will


"not defined in currently in the stack"

parejkoj · 2018-06-15T20:08:51Z

python/lsst/ap/association/assoc_afw_utils.py

+        Minimal schema for DIAObjects.
+    """
+    schema = afwTable.SourceTable.makeMinimalSchema()
+    schema.addField("diaObjectId", type='L')


Please add `doc=' to document what all these fields are.

parejkoj · 2018-06-15T20:19:42Z

python/lsst/ap/association/assoc_afw_utils.py

+    Parameters
+    ----------
+    source_record : `lsst.afw.table.SourceRecord`
+        Input source catalog to modify values of if needed.


"if needed" implies that this is optional. Is it? What is the default value in that case?

parejkoj · 2018-06-15T20:22:47Z

python/lsst/ap/association/assoc_afw_utils.py

+        overwrite_dict['psFluxErr'] = np.sqrt(
+            (psFluxErr / exp_dict['fluxZeroPoint']) ** 2 +
+            (psFlux * exp_dict['fluxZeroPointErr'] /
+             exp_dict['fluxZeroPoint'] ** 2) ** 2)


I don't like that we're coding the error calculation here explicitly. Call something from Calib or PhotoCalib instead. For example, PhotoCalib.instFluxToMaggies(flux, fluxErr) will give you the calibrated flux and fluxErr.

Do calexps or other exposure like objects have a PhotoCalib object? The calib object stored doesn't have an option to convert to maggies. Is there an example of how I can use a PhotoCalib object with an exposure and a catalog?

A more future-proof approach would be to create a PhotoCalib object when you read in the Exposure and extract the Calib and pass it around, instead of the zero-point. This should do it:

fluxMag0 = calib.getFluxMag0() photoCalib = afwImage.PhotoCalib(1.0/fluxMag0[0], fluxMag0[1]/fluxMag0[0]**2, bbox)

That way, once we replace Calib with PhotoCalib (DM-10153), we just have to delete two lines.

Oh, please leave a TODO as well.

parejkoj · 2018-06-15T20:30:14Z

python/lsst/ap/association/assoc_afw_utils.py

@@ -0,0 +1,279 @@
+#


Why not call the file afwUtils.py? It's already in ap.association, so ap.association.assoc_afw_utils seems redundant. Also, files are camelCase:

https://developer.lsst.io/python/style.html#a-python-source-file-name-should-be-camelcase-with-leading-lowercase-and-ending-in-py

parejkoj · 2018-06-15T20:31:45Z

python/lsst/ap/association/assoc_db_sqlite.py

+    ccdVisitSchema, \
+    add_dia_source_aliases_to_catalog, \
+    get_ccd_visit_info_from_exposure, \
+    make_overwrite_dict


Why not just import .afwUtils (per my above filename comment), and then they'll all be namespaced. You'll have to expand that file's __all__.

I'm sort of okay with keeping it this way. It makes it more clear for me when something breaks. Also I'm not sure I want many of these functions exposed for use outside of the context of ap_association.

If you really don't want those things in the namespace, please make a note of that above the __all__ in afwUtils, so that others don't expose it later.

parejkoj · 2018-06-15T20:33:19Z

tests/test_association_db_sqlite.py

@@ -29,13 +29,14 @@
    AssociationDBSqliteTask, \
    AssociationDBSqliteConfig, \
    make_minimal_dia_source_schema
+from lsst.ap.association.assoc_afw_utils import \


Again, probably better to from lsst.ap.association import afwUtils and then you'll have the whole namespace.

See above, I hope that makes sense.

parejkoj · 2018-06-18T19:23:44Z

python/lsst/ap/association/assoc_afw_utils.py

+def convert_dia_source_to_asssoc_schema_and_calibrate(dia_sources,
+                                                      obj_ids=None,
+                                                      exposure=None):
+    """Create aliases in for the input source catalog schema and overwrite


"and overwrite values if requested": what constitutes a request?

parejkoj · 2018-06-18T23:23:32Z

Please rebase squash.

Add afw_dia_schemas Move exposure and overwrite dicts to afw_dia_schemas.py Rename afw_dia_schemas Move all schemas out of assoc_db_sqlite. Finalize assoc_afw_utils Clean up tests and methods to new call signatures. Debug unittests Found bug from previous testing where flux values were not updated properly due to filter ids not being correct. This has been fixed. Clarify doc strings.

Rename references to afwUtils. Fix import in tests_association_db_sqlite Edit obj_id comment docstring.

Use photoCalib return properly. Fix ccdVisitSchema method name Remove bbox from photoCalib object

Fix linter errors Add TODO refering to DM-10153.

parejkoj requested changes Jun 15, 2018

View reviewed changes

parejkoj reviewed Jun 18, 2018

View reviewed changes

parejkoj approved these changes Jun 18, 2018

View reviewed changes

morriscb added 4 commits June 18, 2018 18:28

Rename to afwUtils

6b23305

Rename references to afwUtils. Fix import in tests_association_db_sqlite Edit obj_id comment docstring.

Add PhotoCalib object

07912b1

Use photoCalib return properly. Fix ccdVisitSchema method name Remove bbox from photoCalib object

Add ccdVisitSchema dict to self

cdf86ba

Fix linter errors Add TODO refering to DM-10153.

morriscb force-pushed the tickets/DM-14620 branch from ef3b880 to cdf86ba Compare June 18, 2018 23:29

morriscb merged commit cdf86ba into master Jun 18, 2018

kfindeisen mentioned this pull request Jun 20, 2018

DM-14847: Missing keys in association DB lsst/ap_verify#37

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-14620: Move ap_association DIA schemas their own module #17

DM-14620: Move ap_association DIA schemas their own module #17

morriscb commented Jun 1, 2018

parejkoj left a comment

parejkoj Jun 15, 2018

parejkoj Jun 15, 2018

parejkoj Jun 15, 2018

parejkoj Jun 15, 2018

parejkoj Jun 15, 2018

parejkoj Jun 15, 2018

parejkoj Jun 15, 2018

morriscb Jun 16, 2018

parejkoj Jun 18, 2018

parejkoj Jun 15, 2018

parejkoj Jun 15, 2018

morriscb Jun 16, 2018

parejkoj Jun 18, 2018

parejkoj Jun 15, 2018

morriscb Jun 16, 2018

parejkoj Jun 18, 2018

parejkoj Jun 18, 2018

parejkoj commented Jun 18, 2018

		"ccdVisitSchema"]


		"""Defines afw schemas and conversions for use in ap_association tasks.

DM-14620: Move ap_association DIA schemas their own module #17

DM-14620: Move ap_association DIA schemas their own module #17

Conversation

morriscb commented Jun 1, 2018

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

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

parejkoj commented Jun 18, 2018