DM-8951: Add TE1 and TE2 KPMs to validate_drp #31

wmwv · 2017-02-03T19:42:16Z

No description provided.

parejkoj

See comments. Mostly missing or unclear docstrings, and some tests that should be added.

One of the questions relates to execution speed, and you can feel free to tell me that it's irrelevant in the context of validate_drp.

parejkoj · 2017-02-09T21:12:07Z

etc/metrics.yaml

+      description: Separation distance
+    bin_range_operator:
+      value: "<="
+      description: Are we looking at correlations less than D or greater than D.


Should this be phrased as a question?

parejkoj · 2017-02-09T21:20:46Z

etc/metrics.yaml

+      description: Separation distance
+    bin_range_operator:
+      value: ">="
+      description: Are we looking at correlations less than D or greater than D.


Same phrasing question as above.

parejkoj · 2017-08-05T05:05:42Z

python/lsst/validate/drp/calcsrd/__init__.py

@@ -8,3 +8,4 @@
 from .amx import AMxMeasurement  # NOQA
 from .afx import AFxMeasurement  # NOQA
 from .adx import ADxMeasurement  # NOQA


Do we need # NOQA here? I thought we just ignored the flake8 comments on __init__.py since it's a weird file.

#NOQA for from .tex is in keeping with previous practice for these SRD metrics. Happy to consider overall #NOQA use in future ticket.

parejkoj · 2017-08-05T05:08:33Z

python/lsst/validate/drp/calcsrd/tex.py

+    ----------
+    metric : `lsst.validate.base.Metric`
+        An TE1 or TE2 `~lsst.validate.base.Metric` instance.
+    matchedDataset : lsst.validate.drp.matchreduce.MatchedMultiVisitDataset


matchedDataset of what?

parejkoj · 2017-08-05T05:09:23Z

python/lsst/validate/drp/calcsrd/tex.py

+    filter_name : `str`
+        filter_name (filter name) used in this measurement (e.g., ``'r'``).
+    width : `float` or `astropy.units.Quantity`, optional
+        Width around fiducial distance to include. [arcmin]


width doesn't appear in the argument list.

parejkoj · 2017-08-05T06:14:07Z

python/lsst/validate/drp/util.py

@@ -140,6 +148,32 @@ def sphDist(ra1, dec1, ra2, dec2):
    return dist


+def averageRaFromCat(cat):


Though validate_drp isn't a particularly time-critical operation, I do worry that your use of these "only RA" , "only Dec" functions is slow, given you're throwing away the other average. I see you're using it in the aggregator for TEx: how many values are typically input?

parejkoj · 2017-08-05T06:14:39Z

python/lsst/validate/drp/util.py

+    return meanRa
+
+
+def averageDecFromCat(cat):


At least a one-line docstring for all of these new functions.

parejkoj · 2017-08-05T06:16:23Z

python/lsst/validate/drp/validate.py

+                        outputPrefix=outputPrefix)
+            except RuntimeError as e:
+                print(e)
+                print('\tSkipped plot{}'.format(texName))


Should this be using the lsst.log system to log these errors?

parejkoj · 2017-08-05T06:18:52Z

tests/test_tex.py

+        self.assertFloatsAlmostEqual(exp_all_avg_xip, obs_all_avg_xip, rtol=1e-7)
+        self.assertFloatsAlmostEqual(exp_all_avg_xip_err, obs_all_avg_xip_err, rtol=1e-7)
+
+        exp_rlt2_avg_xip, exp_rlt2_avg_xip_err = 2.8171429e-05, 1.2857143e-06


Please add a note about how these numbers were arrived at.

parejkoj · 2017-08-05T06:21:16Z

tests/test_tex.py

+            select_bin_from_corr(r, xip, xip_err, radius=1, operator=operator.ge)
+        self.assertFloatsAlmostEqual(exp_rge1_avg_xip, obs_rge1_avg_xip, rtol=1e-7)
+        self.assertFloatsAlmostEqual(exp_rge1_avg_xip_err, obs_rge1_avg_xip_err, rtol=1e-7)
+


I think you also want some tests for your correlation_function_ellipticity* functions, and probably for the added utils functions.

Yes. Tests added for correlation_function_ellipticity.

The utility functions that require a MatchedMultiVisitDataset object, a catalog, or ashape object are much more complicated to mock up than seems justified given the simplicity of the functions.

parejkoj · 2017-08-05T18:27:59Z

tests/test_tex.py

+        print("EXP_xip_err: ", repr(exp_xip_err))
+        print("OBS_xip_err: ", repr(obs_xip_err))
+        self.assertFloatsAlmostEqual(exp_r, obs_r, atol=1e-4 * u.arcmin)
+        self.assertFloatsAlmostEqual(exp_xip, obs_xip)


If I'm reading this correctly, you're expecting the correlation function to be identically 0 everywhere?

No, sorry. The tests are a work in progress that I started after submitting the ticket for review and realizing that I needed some tests. I'm hoping to have them fully implemented by Monday.

Ah, sorry! Carry on then. I won't look at things until you tell me to.

parejkoj

Added comments on the new code. I like your approach to the ellipticity tests.

parejkoj · 2017-08-31T23:38:22Z

tests/test_tex.py

+        self.assertFloatsAlmostEqual(exp_rlt2_avg_xip, obs_rlt2_avg_xip, rtol=1e-7)
+        self.assertFloatsAlmostEqual(exp_rlt2_avg_xip_err, obs_rlt2_avg_xip_err, rtol=1e-7)
+
+        exp_rge1_avg_xip, exp_rge1_avg_xip_err = 4.3400000e-05, 1.8000000e-06


Another note about where these numbers come from.

parejkoj · 2017-08-31T23:39:15Z

tests/test_tex.py

+        Which in turn references
+        http://adsabs.harvard.edu/abs/2002A%26A...389..729S
+        """
+        rand_seed = 1238625876


Why this choice for random seed?

parejkoj · 2017-08-31T23:43:13Z

tests/test_tex.py

+        random.seed(rand_seed)
+        # Yes, a million.  N this high and L this large
+        #  gets xip within 1e-7 absolute of the analytic limit.
+        # Takes 25 seconds to run on an early-2015 MacBook Air 2.2 GHz Intel Core i7


Useful comment.

parejkoj · 2017-09-01T00:37:18Z

python/lsst/validate/drp/calcsrd/tex.py

+    """
+    # Translate to 'verbose_level' here to refer to the integer levels in TreeCorr
+    # While 'verbose' is more generically what is being passed around
+    #   for verbosity within 'validate_drp'


Why the extra spaces at the start here?

parejkoj · 2017-09-01T00:46:42Z

python/lsst/validate/drp/calcsrd/tex.py

+    xip = gg.xip * u.Unit('')
+    xip_err = np.sqrt(gg.varxi) * u.Unit('')
+
+    return (r, xip, xip_err)


Might be better to return a namedtuple, so the user doesn't have to keep track of which index is what thing?

I don't disagree, but I've been generically discouraged from using namedtuple in favor of pipe_base struct. But I don't want to invoke pipe_base struct here.

parejkoj · 2017-09-01T00:49:03Z

python/lsst/validate/drp/plot.py

@@ -552,3 +552,62 @@ def plotAMx(amx, afx, filterName, amxSpecName='design', outputPrefix=""):
    plt.tight_layout()  # fix padding
    plt.savefig(plotPath, dpi=300)
    plt.close(fig)
+
+
+def plotTEx(tex, filterName, texSpecName='design', outputPrefix=''):


Also, shouldn't this be plot_TEx?

parejkoj · 2017-09-01T00:50:34Z

python/lsst/validate/drp/plot.py

+
+    ax1.legend(loc='upper right', fontsize=16)
+
+    pathFormat = '{prefix}{metric}_D_{D:d}_{Dunits}.{ext}'


Ah. A comment to that effect would be good (prefix suggests directory to me).

This is based on Bob Armstrong's ellipticity calculation. done in DM-3040

Update TEx page reference to 31. Specify D.

Remove unused magRange option from TEx. Also remove docstring description of non-existent 'width' parameter.

Specifically motivated by ellipticity correlation metrics which ~1e-5.

Adds a 'bin_operator' that specifies whether the metric is for <= given radius value (TE1) or >= given radius value(TE2) Add descriptions to TE1, TE2 parameters.

Remove PLOTting hardcoded stuff from tex.py Comment TE plot lines. Minor whitespace edits.

…ulated.

This matches the log radius binning that was used when computing the correlation function in the first place.

Refactoring initially motivated by desire to make it easier to test the key algorithm without having to construct a MatchedMultiVisitDataset object. Also may allow for future external use from more flexible/direct interface.

Use example test case from TreeCorr to verify wrapper function. TreeCorr itself has much more extensive tests. The test here in validate_drp is to make sure that we're calling TreeCorr correctly in correlate_function_ellipticity and, e.g., getting units correct. Also adds a simple test of select_bin_from_corr.

Make nbins, min_sep, max_sep, verbose keyword arguments passed through to treecorr.GGCorrelation

There's a significant performance penalty in using str keys when looking things up in AFW tables.

Translate verbose=True to Treecorr verbose=2.

Factor of 50 (yes, fifty) increase in performance.

Previously was 'for i, s in unmerate(cat)' Now use extracts entire column at a time.

'calculate_' wasn't a particularly informative prefix. Most functions that return something calculate something.

Had had these reversed from the start of this branch.

Quantities within machine precision of 0 can trigger NaN in numpy.sqrt. Use numpy.lib.scimath.sqrt in complex ellipticity calculation to handle this cleanly.

Also clarify that a random seed in the tests was arbitrarily chosen.

wmwv force-pushed the tickets/DM-8951 branch from 87b4289 to b2095a7 Compare February 16, 2017 17:39

wmwv force-pushed the tickets/DM-8951 branch 5 times, most recently from 0d8020a to d73699e Compare July 31, 2017 20:57

wmwv force-pushed the tickets/DM-8951 branch 2 times, most recently from 769dea2 to 3b1c927 Compare August 5, 2017 00:37

parejkoj reviewed Aug 5, 2017

View reviewed changes

wmwv force-pushed the tickets/DM-8951 branch from 9e76126 to 21078d5 Compare August 5, 2017 14:04

parejkoj reviewed Aug 5, 2017

View reviewed changes

wmwv force-pushed the tickets/DM-8951 branch 2 times, most recently from 403218b to 281eb19 Compare August 9, 2017 13:31

parejkoj reviewed Sep 1, 2017

View reviewed changes

Start base for calculating ellipticity

5bf22c8

This is based on Bob Armstrong's ellipticity calculation. done in DM-3040

wmwv force-pushed the tickets/DM-8951 branch from b369249 to c3b0db1 Compare September 7, 2017 18:31

wmwv added 14 commits September 7, 2017 16:57

Return full concatenated source list in addition to matched catalog.

b3ffce6

Record ellipticity of each source and PSF at source location

dee9bd2

Add utility function to calculate ellipticity from shape.

fee7f61

Add TE1, TE2 specifications to metrics.yaml

90f0336

Update TEx page reference to 31. Specify D.

Add medianEllipticityResidualsFromCat function.

bb4331b

Convert to TExMeasurement class.

0e14a82

Remove unused magRange option from TEx. Also remove docstring description of non-existent 'width' parameter.

Add TE1, TE2 measurements to validation run.

5b5d23e

Make TEx class visible to calcsrd importers.

ab39ce3

Move averageRaFromCat, averageDecFromCat into util.py

6cd0a6b

Generalize to TEx measurement.

ae4834a

Shift to .4g from .4f format for metrics with small and large numbers.

35d7fcf

Specifically motivated by ellipticity correlation metrics which ~1e-5.

Sort out basic TE1, TE2 calculation and bookeeping.

041b531

Fix metrics.yaml TE1, TE2 format and comparator.

7990c58

Adds a 'bin_operator' that specifies whether the metric is for <= given radius value (TE1) or >= given radius value(TE2) Add descriptions to TE1, TE2 parameters.

Define TE1, TE2 as the absolute value of the correlation.

d7bcea3

wmwv added 27 commits September 7, 2017 16:57

Return xip, xip_err as quantities with empty unit.

faf3d94

Store radius, xip, xip_err as extra data blobs.

2c55cf4

Add TE1, TE2 plotTEx function.

77b0b84

Plot TE1, TE2 plots if makePlot in validate.py

05497c2

Remove PLOTting hardcoded stuff from tex.py Comment TE plot lines. Minor whitespace edits.

Document PSF residual ellipticity correlation in list of metrics calc…

43ec2a6

…ulated.

Show both measured and spec value for TE plot.

7a27f87

Display correlation radius on log scale.

308d0f9

This matches the log radius binning that was used when computing the correlation function in the first place.

Remove commented-out code. Reformat whitespace.

b40c2b2

Gracefully handle missing TEx in Job.

423299b

Improve documentation for select_bin_from_corr.

2d60a14

Improve doc for correlation, util, plotTEx.

a32fc44

Make treecorr parameters configurable.

f6f9ca4

Make nbins, min_sep, max_sep, verbose keyword arguments passed through to treecorr.GGCorrelation

Make correlation function ellipticity plot filename a keyword option.

f82d451

Improve calculate_ellipticity shape documentation.

69de17c

Use AFW table schema keys instead of str lookup.

5a9b67c

There's a significant performance penalty in using str keys when looking things up in AFW tables.

Pass through verbose level to TreeCorr.

dd33c18

Translate verbose=True to Treecorr verbose=2.

Collect results in arraays and then store in afw table.

4a9f67a

Factor of 50 (yes, fifty) increase in performance.

Calculate ellipticity from catalog columns instead of looping.

646aaa8

Previously was 'for i, s in unmerate(cat)' Now use extracts entire column at a time.

Rename calculate_ellipticity_* to ellipticty_*

7bc8a38

'calculate_' wasn't a particularly informative prefix. Most functions that return something calculate something.

Minor whitespace reformatting around ellipticity operator.

a65d1cc

Correct e1, e2 definition to real, imag components of ellipticity.

7d54d3c

Had had these reversed from the start of this branch.

Use numpy.lib.scimath.sqrt for ellipticity calculation.

78ff72d

Quantities within machine precision of 0 can trigger NaN in numpy.sqrt. Use numpy.lib.scimath.sqrt in complex ellipticity calculation to handle this cleanly.

Add test functions for ellipticity calculation.

e3a2a7a

Update docstrings to fix e1->real, e2->imag.

d5f0de3

Explain where reference xip test numbers come from.

6e851d0

Also clarify that a random seed in the tests was arbitrarily chosen.

wmwv force-pushed the tickets/DM-8951 branch from bb2e233 to 6e851d0 Compare September 7, 2017 20:59

wmwv merged commit 6e851d0 into master Sep 8, 2017

ktlim deleted the tickets/DM-8951 branch August 25, 2018 06:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-8951: Add TE1 and TE2 KPMs to validate_drp #31

DM-8951: Add TE1 and TE2 KPMs to validate_drp #31

wmwv commented Feb 3, 2017

parejkoj left a comment

parejkoj Feb 9, 2017

parejkoj Feb 9, 2017

parejkoj Aug 5, 2017

wmwv Aug 6, 2017

parejkoj Aug 5, 2017

parejkoj Aug 5, 2017

parejkoj Aug 5, 2017

parejkoj Aug 5, 2017

parejkoj Aug 5, 2017

parejkoj Aug 5, 2017

parejkoj Aug 5, 2017

wmwv Aug 7, 2017

parejkoj Aug 5, 2017

wmwv Aug 6, 2017

parejkoj Aug 6, 2017

parejkoj left a comment •

edited

parejkoj Aug 31, 2017

parejkoj Aug 31, 2017

wmwv Sep 7, 2017

parejkoj Aug 31, 2017

parejkoj Sep 1, 2017

parejkoj Sep 1, 2017

wmwv Sep 7, 2017

parejkoj Sep 1, 2017

parejkoj Sep 1, 2017

		@@ -140,6 +148,32 @@ def sphDist(ra1, dec1, ra2, dec2):
		return dist


		def averageRaFromCat(cat):


		ax1.legend(loc='upper right', fontsize=16)

		pathFormat = '{prefix}{metric}_D_{D:d}_{Dunits}.{ext}'

DM-8951: Add TE1 and TE2 KPMs to validate_drp #31

DM-8951: Add TE1 and TE2 KPMs to validate_drp #31

Conversation

wmwv commented Feb 3, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parejkoj left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parejkoj left a comment •

edited