Review of meas_extensions_psfex changes

bin/psfex.py:

• Please don't use from <package> import * (except in __init__.py files) because it makes it hard to tell where symbols come from and defeats some error checking in linters (and so is disallowed in our coding standards).
• Please add a help string to the parser (I don't personally find description="PSFEX" helpful). If this is a wrapper for an existing well-known PSFEX package then it would help to say that.

python/.../psfex.py:

• The various functions really could use more documentation, including more explanation of what each function does and doxygen markup that describes each parameter.
• Some of the function names seem unclear, such as "makeitLsst". What is "it"? More descriptive names (longer, if need be) would make this code easier to understand. This is especially important for any functions intended to be used outside this module.
• Please include _all_=(...) to specify the public functions in this module. And consider then importing this module in the __init__.py.
• Is it necessary to have code such as that described as "# Hack: I want the WCS so I'll guess where the calexp is to be found" in library routines? This really scares me. We ought to be able to do this job without guessing. Perhaps the function is not being given the right parameters? Note that this same hack appears twice and is the only use for guessCalexp, so some refactoring would probably also help.
• You open using "with pyfits.open(cat) as pf" but never use the resulting file object. I think you can simply eliminate that line and not open the file object.
• The following function has no effect. If you mean _dataType to be a global then you need to "global _dataType" before you set it. Furthermore, it would make your code more robust to use "global _dataType" in any function that uses it, to avoid this kind of error and make it clear you are referring to a variable defined elsewhere. likewise for "_dataTypes". Also the two names are too similar and thus easily confused; I suggest _dataTypeDict for the second.

  def setDataType(t):

  _dataType = _dataTypes[t]

• Please use "as" when catching exceptions, e.g. "except Exception as e:" instead of "except Exception, e:". This eliminates a potential source of confusion when catching multiple exceptions and looks ahead to Python 3. However, in the instance I see the exception isn't used anyway, so "except Exception:" is preferred.
• pyflakes shows 9 instances of variables assigned to that are never used. Some of these are discussed above but most can probably just be deleted.
• nobj = len(xm)
• _dataType = _dataTypes[t]
• fwhmode = np.empty(ncat)
• with pyfits.open(cat) as pf
• next = fields[0].getNext() # number of extensions
• psfstep = prefs.getPsfStep()
• ixy = tab.get("%s.xy" % shape)
• except Exception, e:
• We already have a function called showPsf. What makes yours different? Can you use the other one?
• Some function names use underscore. Is there a compelling reason to do this, becaus our standard calls for camelCase. Even for borrowed code such as compute_fwhmrange it'd be nice to clean up the doc string to meet our standards and rename it to camelCase unless people are strongly expecting its old name.

Speaking of names...I see Field.hh and prefs.hh which predate this ticket, but why the .hh instead of .h? I assume they are C++ wrappers around existing .h files, but if so it'd be nice to say so in the files.

psfexPsfDeterminer.py

• This line is hard to understand: "bool(psfex.Context.REMOVEHIDDEN if False else psfex.Context.KEEPHIDDEN))". I suggest that you set a boolean variable whose name is helpul before calling the function, then use that variable in the funciton call.

prefsImpl.cc

• You added array paramvalues, but don't actually do anything with it except use the current value as a temporary. Wouldn't it suffice to do this, instead:

  string const valueName{values->getAsString(name)};

  argval[i] = const_cast<char *>(valueName.c_str());

PsfexPsf.cc

• Minor point, but I suggest removing these commented-out lines that you added:

  //averagePosition(schema.addField< afw::table::Point<double> >(

  // "averagePosition", "average position of stars used to make the PSF")),

  //table::Key<table::Point<double> > averagePosition;

psfImpl.cc

• Minor point, but please remove the trailing space you added at line 149: "} ".

testPsfexPsf.py

• Minor point, but please remove commented-out line 204: "#self.catalog = SpatialModelPsfTestCase.measure(self.exposure)"

meas_excensions_psfex.table

• Please list all packages directly used by this package, thus boost, fftw (or is it fftw_float?), afw, etc. I know it was already this way, but LSST does have the rule that all direct dependencies are to be listed in the table file.

Russell Owen I agree that a name of hack_clapack gives the wrong impression of what this code is.

There are also review comments on the Github PR.

As for repos.yaml see RFC-75 (recently adopted) for details on how to add a new entry. You also need to make another package depend on this one.

I have addressed most of the comments on this page. Meas_extensions_psfex is now an optional dependency of lsst_apps, and there is a ticket branch in the repo to reflect this change. There are a few changes to bin/psfex.py and python/.../psfex.py which were not made. After discussion with others the best course forward seemed to be putting off changes such as documentation, and refactoring the code into ticket DM-3441. In the short term this will be considered technical debt inherited by the merge, and not part of the merge ticket itself. Comments which are not addressed in this issues are ported to the DM-3441 ticket.

repos.yaml was updated before this went out for review, if you are still not seeing the changes, check that you are up to date with master.

The comments on github have responses addressed to them as well.

fftw package

Excellent update. Your solution of creating two separate directories for the two precisions is a good workaround to the problem that making single and double precision libraries of fftw requires two sequential build+install operations, and our code that handles .cfg.sh files apparently isn't set up to support that.

psfex package

Thank for renaming that one sub-component.

eupspkg.cfg.sh: the new comment is much appreciated. At risk of making you wish you had not added it, please add a space after the initial # in each line. Also, I think there is a minor typo in the last sentence: ", and if it does..."? perhaps you meant ", and if it is then..."?):

    #..."It checks if the eups fftw

    #environment variable is set, and if it does the configuration is

    #set appropriately."

That said, I think the first sentence provides enough explanation by itself.

meas_extensions_psfex package

Thank you for dependencies to the table file. One minor change: please omit swig, since it is not used by the code (unless I missed it somewhere).

psfex.py:

Please fix "from lsst.meas.extensions.psfex.utils import *" on this ticket instead of deferring it. It should only take a few minutes. One technique is to delete that line and see what pyflakes or flake8 linter say is missing, then import those symbols.

prefsImpl.cc

Thank you for the explanation for the paramvalues vector! However...could you please add a space after the comment delimiter, e.g. "// text..." instead of "//text...".

Nate Lust – could you please write a brief description of new functionality you've added to the stack here as part of the release notes at https://confluence.lsstcorp.org/display/DM/Data+Release+Production+WIP+S15+release+notes ? Thanks!