afw tests, including testExecutables.py have been updated for pytest.

Two commits (https://github.com/lsst/afw/commit/77bd61391d1f131d593e80cfcea91dcca9d95cba and https://github.com/lsst/afw/commit/4414ce62550fe818988a1e8adb1b0753077b9837) are failing due to bugs (see comments in the code) but all other tests pass successfully.

Is the ticket ready for review then?

That's Pim Schellart [X]'s call. I updated the ticket so that he knows I'm finished integrating all of the packages.

I just ran this branch and I get two failing tests. One is the problem with testCoord.py discussed on the PR where the true/false sense was switched and should be left alone; the other is in testConvolve.py which seems to be a problem with the python.

Also, I just ran py.test tests/*.py and got huge numbers of failures:

tests/test1079.py ........

tests/testAmpInfoTable.py ....

tests/testAngle.py ..........

tests/testApCorrMap.py ........

tests/testApproximate.py ......

tests/testAstropyTableViews.py ...........

tests/testBackground.py .........................

tests/testBox.py ..................

tests/testCamGeomFitsUtils.py .....

tests/testCameraGeom.py ..s............

tests/testCameraSys.py .....

tests/testCameraTransformMap.py ........

tests/testChebyshevBoundedField.py .F.....

tests/testColor.py ....................

tests/testCoord.py .....F.....................

tests/testCoordinates.py ...............

tests/testCoordptr.py .....

tests/testDetector.py ...........

tests/testDisplay.py ..............

tests/testDistortedTanWcs.py .....

tests/testDs9.py ssssssss..

tests/testEllipse.py ......

tests/testExecutables.py .............................

tests/testExposure.py ...................

tests/testExposureTable.py ......

tests/testFluxFromABMag.py ...

tests/testFootprint1.py ................................................

tests/testFootprint2.py ..........................

tests/testFootprintEllipse.py ...

tests/testFootprintMergeCatalog.py ...

tests/testFunction.py ...............

tests/testFunctor.py .......

tests/testFunctorKeys.py ........

tests/testGaussianProcess.py ...........

tests/testGaussianPsf.py .......

tests/testGeomTestUtils.py ....

tests/testHeader.py ...

tests/testHeavyFootprint.py ..........

tests/testImage.py ........................................

tests/testImageIo1.py ...........

tests/testImageIo2.py ...

tests/testImagePca.py .......

tests/testImagePersistence1.py .....

tests/testImagePickle.py ....

tests/testImageTestUtils.py ....

tests/testInterpolate.py .......

tests/testKernel.py ...................

tests/testKernelImagesForRegion.py ......

tests/testKernelIo1.py .........

tests/testLeastSquares.py ....

tests/testMakePixelToTanPixel.py .....

tests/testMakeWcs.py ........

tests/testMask.py .............................

tests/testMaskedImage.py FFFFFFFFFFFFFFFFFFFFFFFFFFF.F

tests/testMaskedImageIO.py .....................

tests/testMaskedImagePersistence1.py ....

tests/testMatchFits.py .....

tests/testMethods.py .........

tests/testMinimize.py ...

tests/testOffsetImage.py ..........

tests/testOrientation.py .....

tests/testPickles.py ....................

tests/testPolygon.py ..................

tests/testRaWrap.py ...

tests/testRandom1.py .............

tests/testRgb.py ...s......ss.....

tests/testRowColumnStats.py .....

tests/testScaledPlus.py ...

tests/testSchema.py ............

tests/testSeparableXYTransform.py ...

tests/testSimpleTable.py .......................

tests/testSourceMatch.py ......

tests/testSourceTable.py ....................

tests/testSpatialCell.py ...............

tests/testSpline.py ........

tests/testStacker.py ......F.....

tests/testStatBug1697.py ...

tests/testStatClipException1045.py ....

tests/testStatistics.py ..........................

tests/testStatisticsMasked.py ......

tests/testStatisticsOverloads.py ........

tests/testTableAliases.py .........

tests/testTableArchiveImport.py ...

tests/testTableUtils.py .......

tests/testTicket2019.py ...

tests/testTicket2026.py .....

tests/testTicket2162.py ....

tests/testTicket2233.py ...

tests/testTicket2352.py ....

tests/testTicket2707.py .....

tests/testTicket2905.py ...

tests/testTicketDM-433.py ......

tests/testValidPolygon.py ......

tests/testWarpExposure.py F.F.FF.F.F.F..FF..F....

tests/testWarper.py FFFF..

tests/testWcs1.py ............................

tests/testWcs835.py ...

tests/testWcsFitsTable.py ......

tests/testXYTransform.py ...........

It looks like the problem is in aggregate. Running the tests on their own is fine but running them in all together indicates we still have state that is not being tidied up. So I think there needs to be a final pass through to fix up the aggregate problems.

I'll take a look at them in the morning, as well as testCoord and testConvolve.

Hmm, strange. I didn't have any problems with these failing tests. Unfortunately I cannot reproduce now without rebuilding. I'll therefore ask Fred Moolekamp to have a look.

I fixed testCoord.py and pushed it to the branch but I cannot replicate the testConvolve.py failure using either python or py.test. Tim Jenness, do you have a link to the Jenkins build so I can investigate the testConvolve.py failure?

I get the same failures when running "py.test tests/*.py" as reported by Tim Jenness. I'm currently investigating and updating tests.

Python 3.

Just for everyones future knowledge, there were two separate issues causing tests to fail (only when run on the entire directory).

1. numpy.random.seed functions needed to be placed in `setup_module`.
2. Some of the tests changed the mask plane and it needed to be reset to the default in the module teardown.

I pushed the latest changes and Jenkins is rebuilding for python 2 and 3.

Fred Moolekamp the mask plane issue is subtle and I'm glad you figured it out. However, I am uncomfortable with your solution because at the end of the test you are arbitrarily setting the mask dict to hard-coded values. If this doesn't match the correct defaults (e.g. if Mask evolves) then this may mysteriously break other tests in the future.

I suggest, instead, that if a test modifies the mask plane dict, then record the initial dict in setUp and restore it in tearDown. This avoids hard-coding values, though it is not robust against a test failing to restore the original values.

An alternative, or additional thing to consider is to add a method to Mask that restores the mask plane to its default (or at least returns the default dict, which the user can then use to set the dict). I hope Robert Lupton will weigh in on this, since he designed Mask. If such a method existed, then it could be used to restore the default at the end of any test that modifies the mask planes, or to set the default in setUp of all tests classes that are sensitive to the exact mask plane dict (that is safest, but there are so many tests that use masks that it is almost certainly impractical). The need to even consider doing something like this shows an ugly side to pytest.

Hmm... Cleaning up after you've mucked around with things is usually a good thing(tm). To be sure I'm understanding here: the problem is now that altered defaults are bleeding into other tests because pytest runs all the tests together, instead of file-by-file?

I am rather surprised that we don't have a way to reset mask planes to the default.

The Mask object has a static make plane dict. If a test alters it and leaves it altered, then that change is seen by the next test when tests are run by pytest.

We had a similar situation in meas_astrom where a test was changing the open file limit (to 10!) and not restoring it. No problem when run with unittest, but a disaster when run with pytest, and of course the errors seen depended on the order of test execution. Fun, fun fun.

Just to be clear, the change in behavior is because pytest is running all the tests together, not as separate files, each with their own python instance?

Yes. See https://jira.lsstcorp.org/browse/DM-7207?focusedCommentId=52730&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-52730

Russell Owen I was able to isolate the testMask.py module and the missing keys in the mask plane but it took John Swinbank to understand exactly what was going on.

I agree that in the long run it would be best to create a method to reset the MaskU object. Right now the default is set in afw/src/image/Mask.cc at https://github.com/lsst/afw/blob/master/src/image/Mask.cc#L367-L388, which is not wrapped by swig. It seems to me that a new ticket would need to be created to implement this behavior, which should be done.

For now I think it is ok to hardcode the values as getMaskPlaneDict.keys() returns the keys out of order. It isn't hard to check for the value of each key to get the correct order but for a method that should be replaced anyway (with a built in method to reset the mask plane) it doesn't seem worth hacking a solution. Does this make sense?

If I'm understanding this correctly, I would suggest saving the default mask planes (is that possible?) in setUp before they are changed, and then restoring them in tearDown. That way, each test is completely independent, even within each class, which is exactly what we want.

If I'm reading the code correctly (which I might not be), this is more difficult than it should be. I think the only way to access the current keys is through MaskU().getMaskPlaneDict(). I think the method you are suggesting would have to look something like

mask = lsst.afw.image.MaskU()

maskDict = dict(mask.getMaskPlaneDict())

maskPlaneDefaultDict = sorted(maskDict, key=maskDict.get)

I guess that isn't as ugly as I thought it would be but I would still prefer to open a separate ticket to add a method to reset the mask plane. Ideally this new function should be implemented before the defaults are changed, and breaking this test would alert the coder that the default method needs to be created. But I understand that this way of doing things might be uncomfortable to others.

Fred Moolekamp I really feel it's best to go to the trouble of reading the current mask plane dict and restoring it in the one test in question. I assume no other tests do mess with the mask plane dict, but if they do, then repeat the code or isolate it as a test function.

In any case, please file a ticket to add a way to restore default mask dict, and to update the tests when that occurs.

John Parejko Yes. pytest runs a batch of test files in one process, so changes made by one file may be seen by another. This is a major hassle, and explains why many packages have tests that are failing under pytest, and that this failure may depend on test execution order. I like pytest in many ways, but I loathe this misfeature.

Unfortunately the only really safe way to handle mask plane dict under these circumstances is to manually set it to its default value in every test's self.setUp or at least self.setUpModule or setup_module. However, we don't have code that can do that (yet) and even if we did, it would mean cluttering up hundreds of tests in dozens of packages. Fred Moolekamp is taking a pragmatic approach that dramatically reduces clutter: if a test knowingly modifies Mask's dict, then it restores it in self.tearDown. I have a minor quibble with the way that Fred Moolekamp is restoring the dict, but I think his overall approach is a reasonable compromise between safety and clutter.

Fred Moolekamp I think your snippet looks great. However, maskPlaneDefaultDict is a list of mask planes in numeric order, not a dict, so the name is confusing. You could also combine the first two lines (and eliminate the dict(...) if you want, though no big deal). I.e. this works:

def setUp(self):

    maskPlaneDict = lsst.afw.image.MaskU().getMaskPlaneDict()

    self.maskPlanes = sorted(maskDict, key=maskDict.__getitem__)  # list of mask plane names in order 0, 1, ...

I think it would be significantly clearer than what you have now. I hope you will consider adopting it.

Also, it is non-pythonic that getMaskPlaneDict() cannot be called directly on MaskU. That would be pretty easy to fix in the SWIG (as a separate ticket).

Putting the "reset to default" bit in tearDown is the safe approach. Each test should restore the state of global things in tearDown every time. "If a test knowingly modifies Mask's dict, then it restores it in self.tearDown." is exactly the right approach.

I agree with John Parejko. If a test modifies some internal state it should reset it when it ends. I don't understand Russell Owen's objection to having to restore state after tests complete. Being able to run multiple test modules in a single test runner is a feature not a bug as far as I'm concerned. It makes it really easy to run batches of tests from the command line (without relying on sconsUtils magic) and even lets me run every test in every package all at once to get a quick coverage report.

The scary word to me is "knowingly". Tests may call code that adds mask planes without the test realizing this. Before pytest any such effect was localized to the test file in question. Now it spreads to all tests in a package (and if we ever run pytest on more than one package at a time, it will spread even farther).

The only truly robust approach to isolate a test file from other test files is to restore the mask in setUp or setUpModule. And I think that is more work than it is worth. Unfortunately.

We are going to have to agree to disagree here. If the test changes internal global state "unknowingly" then that's something I'd really like to understand before it bites us hard during a production run where some task does something that causes some confusing "action at a distance" failure later in the task.

Using the code suggested by Russell Owen I'll store the current defaults in setUp and restore them in tearDown. I think this is the last issue preventing this ticket from being merged. I'll update this ticket tomorrow once it passes all of the tests. Tim Jenness, would you mind if I made you the reviewer? I'll also open a ticket to make the `setInitMaskBits` method public but recommend that it isn't implemented until after the swig/pybind11 RFC is resolved.

I'm also curious about the design of the mask plane. I'm not sure I like the global mask plane being modified as opposed to a single instance and I'd be interested to know the reason for this design in the first place.

It is fairly normal for code to add mask planes and I personally have no idea what code does this. Perhaps that is unlikely to break other tests, but it disturbs me nonetheless.

Global state is always an interesting decision. Yes, hit me with the review but it's going to be a busy two days coming up so I might not be able to get through all 100 tests quickly.

I believe Mask is on the list somewhere of things to redesign for this very reason.

Thanks Tim Jenness, I just pushed the changes discussed above and the ticket should be ready for review.

Fred Moolekamp can you rebase the ticket branch so that I can get a recent fix for Python 3 for my testing?

There are also a couple of bad merge commits on that ticket branch that a rebase should clear out.

I'm resolving conflicts now. I'll let you know when I push the rebased ticket branch.

I'll review testLeastSquares.py and later (alphabetically)

There are many instances of defining a small function (often named tst and usually a single line) then then calling it with self.assertRaises(...). These should all be replaced with:

with self.assertRaises(...):

    body of test function (usually just one line)

I suggest searching for ^ *self.assertRaises to find these

Some of the tests still assume they are being run from a directory that has a tests subdirectory. Please fix them to use _file_ or getPackageDir. You can easily reproduce by changing to the directory above afw and running py.test afw/tests/*.py.

assertClose is deprecated; please use assertFloatsAlmostEqual (from lsst.utils.tests.TestCase) for assertAlmostEqual (from unittest).

If you are willing to take a cleanup pass on the namespace for numpy it would be appreciated. Our standard is import numpy as np

Please take a cleanup pass on two issues:

• Fix flake8 errors. At least one file has a line that is too long, and probably more (this is a common error introduced by autopep8: when it corrects the indentation of continuation lines it ignores length errors this indentation introduces). It's also worth looking for more serious errors (I have not done that).
• Put the imports into the correct order and grouping (e.g. testWcs1.py). future adds imports at the top, which is not usually where they belong. future is a third party package, so its imports belong in the 2nd group (1st group is standard library, 2nd group is third party packages, 3rd group is LSST software).

Overall testLeastSquares.py and later look great, and I applaud the heroic effort of those that contributed to updating these tests. I added a few minor comments to github and the suggestions above.

I am offering to help with some of the further cleanup, if desired.

Heroic indeed. Wow. I made it to the least squares test. Lots of little comments but it's wonderful. I am going to trust that you implement all the review fixes so please don't ask me to review this PR again...

I don't understand how the Mask tests broke. Were they broken in the old code and it was only revealed by Fred's work? Or is it something about the new test framework? I thought that I'd been pretty careful about cleaning up (but the Mask design is a mess – I inherited it, and made it work, but I'm not sure that there's where I'd start from scratch)

Robert Lupton it depends on what you mean by broke. When run individually the tests work fine (this is how the old tests were run). Now, pytest can be run on a batch of files and it doesn't clear the global namespace between the execution of each test module. I agree with Russell Owen that this is not ideal but it is the way that pytest works. Since testMask.py and testMaskedImageIO.py clear the maskPlaneDict to run their tests, any tests run after them that require the use of a mask plane fail because the defaults have been replaced. Does that clarify things?

Russell Owen, Tim Jenness thanks for reviewing this ticket (and for doing it so quickly). I'll work on making the corrections today. And thanks to Pim Schellart [X], John Parejko, and Nate Lust for helping with this ticket. 132 commits is really a team effort!

Tim Jenness I found the problem in testConvole.py. Myself (and anyone) who already had an afw branch before the tests were renamed (kernel.py->testKernel.py) at the conference had an old kernel.pyc file hanging around in the tests directory. Since *.pyc files are listed in .gitignore they were never removed and thus import kernel worked for us. I'm not sure why the tests passed in Jenkins, but I successfully built the ticket in Jenkins several times this week.

Yes, that's clear thanks (and congratulations on finding it).

I think extending the Mask API to reset the default set of mask planes would be appropriate. Actually I don't think I'd add it to Mask – it's already a bad idea for the default mask planes to be in C++. I'd add a python utility function to set this, and probably make it configurable via pex_config

R

Robert Lupton, I see your point but I think that if a python utility function is created we are still left with the problem of changing the default mask planes in multiple places (if they ever change). At the very least the object that defines the default masked planes should be public, which it currently isn't.

Tim Jenness Russell Owen The changes are complete and the tests passed on Jenkins. It is ready to merge but I wanted to give you both one last chance to review the new commit after the review if you like. I did have to modify testTableArchives.cc to allow the all of the tests to run from an arbitrary directory, which is now possible. If I haven't heard back by tomorrow afternoon I'll merge the changes.

I had a quick look this afternoon and everything looked good. Did you run a python 3 jenkins test as well?

I'm not sure how to test on python 3 in Jenkins. Typically I was taught to use lsst_apps as the product, but that fails when trying to build mysqlpython (see https://ci.lsst.codes/job/stack-os-matrix/15103/label=centos-7,python=py3/console).

Change the product to "afw", check the box to disable the demo.

Jenkins build passed (https://ci.lsst.codes/job/stack-os-matrix/15104/).

Robert Lupton, I see your point but I think that if a python utility function is created we are still left with the problem of changing the default mask planes in multiple places (if they ever change). At the very least the object that defines the default masked planes should be public, which it currently isn't.

The "multiple places" being one C++ and one python function? True. So that's an argument for writing the utility function in C++ so it can be called by the Mask ctor. Actually, it is in C++ (setInitMaskBits() in an anon namespace so not part of the API — although that assumes an empty dict and doesn't empty it first).

Incidentally, I don't like using swig to mess with the API – if you want to extend the API, please extend it in C++. One reason for people being confused by swig is the tendency for some of our wrappers to be full of %extend blocks instead of letting swig do its thing. I hope that this isn't going to complicate the pybind11 switch too much (if it happens)

The changes look good to me. Thank you for being willing to take the time for the cleanup pass.

Sure, thanks for helping with the review. I think this ticket is finally done!