DM-14806: bring demo to standards #20

SimonKrughoff · 2018-07-13T01:46:27Z

This also takes care of: DM-15044

timj

Minor things need fixing but it looks great. Thank you.

timj · 2018-07-13T03:56:26Z

tests/SConscript

@@ -0,0 +1,3 @@
+# -*- python -*-
+from lsst.sconsUtils import scripts
+scripts.BasicSConscript.tests()


Can you add pyList=[] argument so auto file detection is turned on? Then add a setup.cfg so that we can flake8 the python files.

Done, I think.

timj · 2018-07-13T03:56:56Z

tests/test_demo.py

+
+import unittest
+
+from lsst.utils import tests


The idiom we use is import lsst.utils.tests and then be explicit below.

timj · 2018-07-13T03:57:22Z

tests/test_demo.py

+
+from lsst.utils import tests
+
+executable_dir = 'bin'


Please determine this relative to this test file so that the tests can run from outside the demo root.

You mean determine the absolute path, right?

I mean:

executable_dir = os.path.join(os.path.dirname(__file__), os.path.pardir, "bin")

You might need to include os.path.abspath()and os.path.normpath() as well.

timj · 2018-07-13T03:59:34Z

ups/lsst_dm_stack_demo.build

@@ -0,0 +1,2 @@
+@LSST BUILD@ &&


Is this file needed? I don't have one in daf_butler and everything works.

Cargo culted from ci_lsst.

Drop it. We don't use the build files any more.

timj · 2018-07-13T04:00:25Z

ups/lsst_dm_stack_demo.table

@@ -0,0 +1 @@
+setupRequired(obs_sdss)


You need to add utils dependency (you use it in the tests) along with python and sconsUtils.

I think I did this.

timj · 2018-07-13T04:01:26Z

bin/export-results

            vecs.append(v)

        for vals in zip(*vecs):
-            print(' '.join([str(el) for el in vals]))
+            print(' '.join(['{0:.12g}'.format(el) if issubclass(el.dtype.type, np.floating) else str(el) for el in vals]))


Might be worth adding a comment before this line

timj · 2018-07-13T16:03:11Z

SConstruct

@@ -0,0 +1,3 @@
+# -*- python -*-
+from lsst.sconsUtils import scripts
+scripts.BasicSConstruct("lsst_dm_stack_demo")


Add disableCc since we don't need a C++ compiler.

timj

This looks good. I have two comments:

check_astrometry is not being run but we suggest in the README that it should be run. If it is meant to be run we should run it.
Now that we are building with scons we should fix up the shebang and move the python files to bin.src.
The python scripts in bin/ are not picked up by flake8 because they do not have .py extensions. Moving them to bin.src requires we put the .py extensions on so that will solve this problem.

timj · 2018-07-13T18:01:49Z

.travis.yaml

+language: python
+matrix:
+  include:
+    - python: '2.7'


Remove the 2.7 and 3.5 stuff.

sudo: false language: python matrix: include: - python: '3.6' install: - pip install flake8 script: flake8

is sufficient.

timj · 2018-07-13T18:02:09Z

.travis.yaml

+    - python: '3.6'
+      install:
+      - pip install flake8
+      script: flake8


missing newline.

timj · 2018-07-13T18:03:02Z

setup.cfg

+addopts = --flake8
+flake8-ignore = E133 E226 E228 N802 N803 N806
+    processCcd.py ALL
+    andConfig.py ALL


New line needed.

timj · 2018-07-13T18:04:15Z

tests/test_demo.py

+import lsst.utils.tests
+from lsst.utils import getPackageDir
+
+package_root = getPackageDir('obs_test')


It's open to debate whether it's best to use getPackageDir or use this test file location as the reference. I tend to prefer the latter but I'm fine with the former if that's the way you want to go.

I decided to leave it the way it is. If you hate it, let me know.

It had better be asking for lsst_dm_stack_demo package location though 😉 -- although as you've written it you are making my case for using __file__.

timj · 2018-07-13T18:04:38Z

ups/lsst_dm_stack_demo.build

@@ -0,0 +1 @@
+build_lsst @PRODUCT@ @VERSION@ @REPOVERSION@


Remove this file.

timj · 2018-07-13T18:05:24Z

tests/test_demo.py

+
+
+if __name__ == "__main__":
+    lsst.utils.tests.init()


This is not necessary. We are not doing any tests that leak files or memory (and it's not run by pytest because there is no setup_module equivalent.

timj · 2018-07-13T18:14:11Z

tests/test_demo.py

+        """Test demo"""
+        self.assertExecutable("demo.sh",
+                              root_dir=executable_dir,
+                              args=["--small"],


Is it okay to use --small here? How do we ever test with large?

I'm going to leave as is for now as I don't know how this will be dealt with down the road.

timj · 2018-07-13T18:45:48Z

setup.cfg

@@ -0,0 +1,10 @@
+[flake8]
+max-line-length = 110
+ignore = E133, E226, E228, N802, N803


timj · 2018-07-13T19:06:45Z

Probably should update the REAME as well.

timj · 2018-07-13T22:25:34Z

bin.src/SConscript

@@ -0,0 +1,3 @@
+# -*- python -*-
+from lsst.sconsUtils import scripts
+scripts.BasicSConscript.shebang()


Add newline

This test just tests that execution happens without failure.

timj · 2018-07-17T22:01:38Z

@SimonKrughoff I rebased this branch and did a test build on Jenkins. It failed the astrometry check:

1112 sources in camcol : 4
1112 Sources in reference visit : 4192
1264 sources in camcol :  4
Visit : 6377 750 matches found
Median value of the astrometric scatter - all magnitudes: 169.760284819 mas
Astrometric scatter (median) - mag < 19.5 : 102.698862873 mas
Median astrometric scatter 102.7 mas is larger than reference : 100.0 mas

See https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/28217/pipeline/46

timj · 2018-07-17T22:18:15Z

It also looks like check_astrometry tries to make a plot and this fails because $DISPLAY is not set on Jenkins.

RobertLuptonTheGood · 2018-07-18T13:33:05Z

Plots should all be controlled by lsstDebug (for now), so this sounds like an easy fix.

timj · 2018-07-18T13:36:24Z

The plots aren't protected by anything. It always seems to make the PNG. The fix is easy in that we just need to run with a command line option --no-plot or something. More important is that the check_astrometry routine doesn't return any information on pass/fail so the script exit status can't be reported to the test harness.

wmwv · 2018-07-18T13:41:08Z

The key issue is that the default matplotlib on the Jenkins nodes assumes a backend that the default Jenkins nodes don't actually have.
The import matplotlib.pylab as plt line should be moved from a module-level import to be within def plotAstrometry function.
main could be updated to not plot by default. We're not actually looking at or saving the plot.

def main(repo, runs, fields, ref, ref_field, camcol, filter, plot=False):
    """Main executable.

    """

    struct = loadAndMatchData(repo, runs, fields, ref, ref_field, camcol, filter)
    mag = struct.mag
    dist = struct.dist
    match = struct.match
    checkAstrometry(mag, dist, match)
    if plot:
        plotAstrometry(mag, dist, match)

wmwv · 2018-07-18T13:43:16Z

Plots should all be controlled by lsstDebug (for now), so this sounds like an easy fix.

Can you remind me where this policy is described and if there is a snippet of the recommended wrapper code to implement this?

I agree with using lsstDebug for plotting in all of the science pipelines algorithmic code, but it's less clear to me that things like this demo that are trying demonstrate the full capabilities and meant to be run both in test environments and by an end user should require using lsstDebug to set up a plot.

RobertLuptonTheGood · 2018-07-18T14:22:33Z

I was lazy and didn't read the commit -- I expected that useful plots would get added to the Task code (using lsstDebug) and enabled by a command line option, rather than being directly added to the demo framework.

timj · 2018-07-18T16:34:46Z

@wmwv I think your proposal agrees with mine and looks fine. We do need to set exit status if the astrometry test fails, although I'm not sure if anyone has noticed that this test is failing and what the right answer really is (and whether it matters that @SimonKrughoff changed the filter for the astrometry test to make it work with --small).

Jenkins doesn't really like plotting things.

timj · 2018-07-18T17:36:22Z

I've made some tweaks and it now runs fine on Jenkins and fails because 102 > 100. https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/28221/tests

It's for someone else to decide that 102 mas is fine.

jdswinbank · 2018-07-18T18:39:24Z

If I understand the above (having only just skimmed it), this doesn't represent a regression in the astrometry — we were already hitting 102 mas, it's just that we didn't previously notice because we weren't running the test. Given that, I don't think we should wait for a judgement on whether that's acceptable before merging: set the test threshold so that it passes, and at least that means we'll catch it if gets worse. If you're nervous about the absolute value, make a ticket to request that @TallJimbo or @ebellm take a look.

timj · 2018-07-18T18:48:14Z

This is indeed the first time we've set exit status and would notice a problem. Digging deeper the difference is the change in filter that @SimonKrughoff had to add to the check program to make it work with --small. With the r filter:

CameraMapper INFO: Loading exposure registry from /Users/timj/work/lsstsw3/build/lsst_dm_stack_demo/input/registry.sqlite3
777 sources in camcol : 4
777 Sources in reference visit : 4192
892 sources in camcol :  4
Visit : 6377 576 matches found
Median value of the astrometric scatter - all magnitudes: 103.87098207092725 mas
Astrometric scatter (median) - mag < 19.5 : 85.26176053780722 mas
Ok.

and with the i filter currently being used:

CameraMapper INFO: Loading exposure registry from /Users/timj/work/lsstsw3/build/lsst_dm_stack_demo/input/registry.sqlite3
1112 sources in camcol : 4
1112 Sources in reference visit : 4192
1264 sources in camcol :  4
Visit : 6377 750 matches found
Median value of the astrometric scatter - all magnitudes: 169.76028481841527 mas
bin.src/check_astrometry.py:226: RuntimeWarning: invalid value encountered in less
  idxs = np.where(np.asarray(mag) < good_mag_limit)
Astrometric scatter (median) - mag < 19.5 : 102.6988628735584 mas
Median astrometric scatter 102.7 mas is larger than reference : 100.0 mas

I'll force this to 105mas for this filter so it will pass.

timj · 2018-07-18T19:31:39Z

With these changes the demo runs on Jenkins as a standard eups package and passes tests. It now doesn't work the old way (it could do with some tweaking to the demo script) but it's not clear we need it to since we will be removing the special case running of demo in Jenkins.

timj · 2018-07-18T19:32:55Z

README

@@ -14,7 +14,7 @@ Usage
 To run the example, do::

 	$ source $LSST_HOME/loadLSST.sh
-	$ setup obs_sdss
+	$ setup -r .


We need to decide whether the instructions then say "run scons" or whether they stay as they are and we make demo.sh look in ../bin.src/ for the Python code.

timj approved these changes Jul 13, 2018

View reviewed changes

timj reviewed Jul 13, 2018

View reviewed changes

SimonKrughoff force-pushed the tickets/DM-14806 branch from 5cb6b1e to 72f3d9c Compare July 13, 2018 17:57

timj approved these changes Jul 13, 2018

View reviewed changes

timj reviewed Jul 13, 2018

View reviewed changes

bin.src/SConscript

@@ -0,0 +1,3 @@

# -*- python -*-

from lsst.sconsUtils import scripts

scripts.BasicSConscript.shebang()

Copy link

Member

timj Jul 13, 2018

Choose a reason for hiding this comment

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

Add newline

SimonKrughoff force-pushed the tickets/DM-14806 branch from 1f3dbf9 to 6961d67 Compare July 13, 2018 23:17

SimonKrughoff added 12 commits July 17, 2018 14:28

Add build file

39c8da9

Add test

629537d

This test just tests that execution happens without failure.

Add files needed by eups

fd8b009

Update .gitignore

a78e8f2

Turn on Flake8

02dbb7b

Rename bin to bin.src

db25963

Add .py extention to python utilities

ad431c7

Update demo.sh for change in utility extention

84f2e64

Run the astrometry check in the unit test

ea2646e

Modify to work with --small

d1dd51a

Make pass for flake8

26a9ded

Update README for rename

4800e17

timj force-pushed the tickets/DM-14806 branch from 6961d67 to 4800e17 Compare July 17, 2018 21:29

timj added 2 commits July 18, 2018 09:45

Defer matplotlib import and do not plot by default

a953643

Jenkins doesn't really like plotting things.

Add newline to end of travis file and SCons file

4897f00

timj force-pushed the tickets/DM-14806 branch from efead96 to 4897f00 Compare July 18, 2018 16:45

Set exit status to 1 if the astrometric error is too high

1d8b8ab

timj added 2 commits July 18, 2018 11:52

demo.sh should stay in bin/ since it does not change shebang

9ecc6b8

Allow i filter to have higher astrometric error

229c8d3

timj reviewed Jul 18, 2018

View reviewed changes

SimonKrughoff added 3 commits August 9, 2018 02:32

Add stop gap script to allow jenkins to run

44d36a8

Call afterburner from bin.src

841b0d2

Modify .gitignore to honor shell scripts in bin

13a6dfb

SimonKrughoff merged commit 13a6dfb into master Aug 9, 2018

ktlim deleted the tickets/DM-14806 branch August 25, 2018 06:15

		@@ -0,0 +1 @@
		setupRequired(obs_sdss)

DM-14806: bring demo to standards #20

DM-14806: bring demo to standards #20

Conversation

SimonKrughoff commented Jul 13, 2018

timj 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

timj Jul 13, 2018 • 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

timj 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

timj commented Jul 13, 2018

Choose a reason for hiding this comment

timj commented Jul 17, 2018

timj commented Jul 17, 2018

RobertLuptonTheGood commented Jul 18, 2018 via email

timj commented Jul 18, 2018

wmwv commented Jul 18, 2018 • edited

wmwv commented Jul 18, 2018

RobertLuptonTheGood commented Jul 18, 2018 via email

timj commented Jul 18, 2018

timj commented Jul 18, 2018

jdswinbank commented Jul 18, 2018

timj commented Jul 18, 2018

timj commented Jul 18, 2018

timj Jul 18, 2018 • edited

Choose a reason for hiding this comment

timj Jul 13, 2018 •

edited

wmwv commented Jul 18, 2018 •

edited

timj Jul 18, 2018 •

edited