DM-14253: Modernize python in meas_base and meas_algorithms #119

r-owen · 2018-05-01T02:07:08Z

No description provided.

Remove `from __future__ import...` Remove use of future module, including `with_metaclass`

timj

Fro the try blocks catching Exception do we know anything more specific or is it really anything is as bad as anything else?

timj · 2018-05-03T19:47:25Z

python/lsst/meas/algorithms/coaddPsf/coaddPsfContinued.py

 from .coaddPsf import CoaddPsfControl
 from lsst.pex.config import makeConfigClass

-makeConfigClass(CoaddPsfControl, "CoaddPsfConfig")
+CoaddPsfConfig = makeConfigClass(CoaddPsfControl, "CoaddPsfConfig")


How did this work before?

I'm not sure it did work before. I suspect it was a no-op.

r-owen · 2018-05-03T19:57:15Z

In most cases the bare exceptions were in plotting code, where it seemed that a "best effort" was intended, instead of expecting some particular exception. In cases where that was not true I tried to be more careful.

r-owen · 2018-05-03T20:06:48Z

That said, changing except: to except Exception: is sleazy and could be masking lots of bugs. The best I can say is this change didn't make the code any worse. It's not easily testable. I think doing better is out of scope for this ticket. I can file a ticket to have a closer look if you like, but I fear it may never get worked on.

timj · 2018-05-03T20:12:20Z

Let it go. It's easy to find them if we ever feel the need to look again.

parejkoj

I recall when I did this that there were a handful of builtins imports hidden below other imports: did you do an all file search for future and builtins?

parejkoj · 2018-05-03T19:58:37Z

tests/test_dynamicDetection.py


-from builtins import zip



Did this leave 3 empty lines here?

Not that I can see in my copy. I wonder why this got hidden as outdated? I have not pushed anything since May 1

parejkoj · 2018-05-03T20:04:25Z

python/lsst/meas/algorithms/pcaPsfDeterminer.py

@@ -39,7 +39,7 @@
 from .spatialModelPsf import createKernelFromPsfCandidates, countPsfCandidates, \
    fitSpatialKernelFromPsfCandidates, fitKernelParamsToImage
 from .pcaPsf import PcaPsf
-from . import utils as maUtils
+from . import utils as mautils


Why change this name to non-camelCase?

Because flake8 complained

Complained about what? maUtils is follows our other camelCase renamings better.

warning flake8: N812 lowercase 'maUtils' imported as non lowercase

Can you instead just do from . import utils instead of renaming it?

parejkoj · 2018-05-03T20:05:17Z

python/lsst/meas/algorithms/pcaPsfDeterminer.py

                                                      normalize=normalizeResiduals,
                                                      showBadCandidates=showBadCandidates,
                                                      variance=True)
-                        except:
+                        except Exception:


Can you look at what showPsfCandidates could raise, so as to make this exception more specific?

I don't think that is reasonable. It's 200 lines of code, with no documentation as to what exceptions it might raise. I admit that many of those lines are contained in their own try/except Exception blocks, but it's still a big job to try to figure out all the possible errors it might raise.

parejkoj

I marked the except Exception blocks that I think could use a little more investigation (they only call one or two things). Give them each a minute or two of looking at the try-ed code to see if you can narrow down the list.

I don't like the others, but I've filed DM-14310 to deal with it more broadly.

parejkoj · 2018-05-03T20:27:45Z

python/lsst/meas/algorithms/utils.py

@@ -224,7 +224,7 @@ def showPsfCandidates(exposure, psfCellSet, psf=None, frame=None, normalize=True
                # residuals using spatial model
                try:
                    subtractPsf(psf, im, xc, yc)
-                except:
+                except Exception:


Please check if there's an obvious exception(s) raised by subtractPSf here.

Here is the full documentation for subtractPsf:

/** * Subtract a PSF from an image at a given position */ template<typename MaskedImageT> double subtractPsf(afwDetection::Psf const& psf, ///< the PSF to subtract MaskedImageT *data, ///< Image to subtract PSF from double x, ///< column position double y, ///< row position double psfFlux ///< object's PSF flux (if not NaN) )

I think this is a clear case where except Exception: is actually reasonable. This means "make a best effort to subtract the PSF and continue if it fails". Fortunately this is plotting code.

parejkoj · 2018-05-03T20:28:06Z

python/lsst/meas/algorithms/utils.py

@@ -246,7 +246,7 @@ def showPsfCandidates(exposure, psfCellSet, psf=None, frame=None, normalize=True

                    try:
                        noSpatialKernel = psf.getKernel()
-                    except:
+                    except Exception:


Please check if there's an obvious exception(s) raised by psf.getKernel() here.

It is C++ code, with different versions for different PSF classes. And none of it documents any exceptions that I can find.

parejkoj · 2018-05-03T20:31:18Z

tests/test_psfDetermination.py

-                    I = I0*psfVal(ix, iy, x + dx, y + dy, sigma1, sigma2, b)
-                    Isample = rand.poisson(I) if addNoise else I
+                    Ival = I0*psfVal(ix, iy, x + dx, y + dy, sigma1, sigma2, b)
+                    Isample = rand.poisson(Ival) if addNoise else Ival


Is intensity a better name here?

parejkoj · 2018-05-03T20:31:42Z

tests/test_psfDetermination.py

@@ -258,7 +258,7 @@ def subtractStars(self, exposure, catalog, chi_lim=-1):
            if bbox.contains(afwGeom.PointI(int(xc), int(yc))):
                try:
                    measAlg.subtractPsf(psf, subtracted, xc, yc)
-                except:
+                except Exception:


Please check if there's an obvious exception(s) raised by subtractPSf here (as above).

I agree that a unit test is a very nasty spot to ignore errors. I tried leaving out the try/except to see what got raised in the and nothing did, so I removed it.

r-owen · 2018-05-03T20:33:22Z

I did indeed search for "future" and "builtins" and removed all associated imports.

parejkoj

Please check that the doxygen builds without errors/warnings after this change.

parejkoj

A few comments warranting further investigation. I'll trust your judgment on those.

parejkoj · 2018-05-03T20:41:29Z

Further thought about except Exception as e: pass blocks in plotting code: at bare minimum, add log.warn("Raised %s: %s"%(type(ex).__name__, e) before the pass, so we at least have a log of what was raised.

r-owen · 2018-05-03T21:06:29Z

I fixed one try/except Exception in a unit test and am declaring that enough. The plotting code is riddled with them and I think it will take some care and exercising the code to understand where exceptions should be logged or their scope reduced.

both with scons and github Travis

This eliminates a source of warnings from the Python interpreter (e.g. unrecognized escape sequence \p)

r-owen added 2 commits April 30, 2018 19:04

Remove python 2 support

e6df789

Remove `from __future__ import...` Remove use of future module, including `with_metaclass`

Remove obsolete file

eeb2509

timj mentioned this pull request May 3, 2018

DM-14102 makePsfCandidates is now its own Task #118

Merged

timj approved these changes May 3, 2018

View reviewed changes

parejkoj reviewed May 3, 2018

View reviewed changes

parejkoj approved these changes May 3, 2018

View reviewed changes

r-owen added 7 commits May 3, 2018 14:10

Fix pep8 warnings

9d97323

Enable automatic flake8 testing

9c23eea

both with scons and github Travis

Remove unneeded try/except from test_psfDeterm

b2b4832

Fix a Doxygen error

96d8c8b

Change Doxygen \ to @

022217f

This eliminates a source of warnings from the Python interpreter (e.g. unrecognized escape sequence \p)

Remove (object) from Python class definitions

2e405ce

Remove "shbang" lines from unit tests

04672f3

r-owen force-pushed the tickets/DM-14253 branch from 9d4cce5 to 04672f3 Compare May 3, 2018 21:12

r-owen merged commit 4b67208 into master May 3, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-14253: Modernize python in meas_base and meas_algorithms #119

DM-14253: Modernize python in meas_base and meas_algorithms #119

r-owen commented May 1, 2018

timj left a comment

timj May 3, 2018

r-owen May 3, 2018 •

edited

r-owen commented May 3, 2018

r-owen commented May 3, 2018 •

edited

timj commented May 3, 2018

parejkoj left a comment

parejkoj May 3, 2018

r-owen May 3, 2018

parejkoj May 3, 2018

r-owen May 3, 2018

parejkoj May 3, 2018

r-owen May 3, 2018

parejkoj May 3, 2018

r-owen May 3, 2018

parejkoj May 3, 2018

r-owen May 3, 2018

parejkoj left a comment

parejkoj May 3, 2018

r-owen May 3, 2018 •

edited

parejkoj May 3, 2018

r-owen May 3, 2018

parejkoj May 3, 2018

parejkoj May 3, 2018

r-owen May 3, 2018 •

edited

r-owen commented May 3, 2018

parejkoj left a comment •

edited

parejkoj left a comment

parejkoj commented May 3, 2018 •

edited

r-owen commented May 3, 2018

DM-14253: Modernize python in meas_base and meas_algorithms #119

DM-14253: Modernize python in meas_base and meas_algorithms #119

Conversation

r-owen commented May 1, 2018

timj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-owen May 3, 2018 • edited

Choose a reason for hiding this comment

r-owen commented May 3, 2018

r-owen commented May 3, 2018 • edited

timj commented May 3, 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

parejkoj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-owen May 3, 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

r-owen May 3, 2018 • edited

Choose a reason for hiding this comment

r-owen commented May 3, 2018

parejkoj left a comment • edited

Choose a reason for hiding this comment

parejkoj left a comment

Choose a reason for hiding this comment

parejkoj commented May 3, 2018 • edited

r-owen commented May 3, 2018

r-owen May 3, 2018 •

edited

r-owen commented May 3, 2018 •

edited

r-owen May 3, 2018 •

edited

r-owen May 3, 2018 •

edited

parejkoj left a comment •

edited

parejkoj commented May 3, 2018 •

edited