msimet/Stile — pull request #16 by msimet (2014-06-30 21:26:55+00:00)
Title: #8 Code review of master + finishing unit tests

Hello all,

Here's a PR for issue #8, which was intended to finish up the unfinished unit test suite, as well as act as a place for people to review the code on master that hasn't already been reviewed. As part of getting the unit test suite working, in addition to some other concerns driven by branches #7 and #14, there's actually a lot of updated code in here, so going through this PR may take some time.

Also, since this is intended to be a code review of master, you might not want to use the "files changed" interface as usual, but instead look at the whole code as of the latest commit--right now that should be [this link](https://github.com/msimet/Stile/tree/8b48d59dd2457b2935de2bd6b93e38dc2a48f794). The main code is in the `stile/` directory, tests in `tests/`, and example code in `examples/`. Or just pull branch #8. That'll be a little annoying for line-by-line comments, I'm afraid, though I suspect most of the code is going to show up in the "files changed" pane anyway since there were a lot of small tweaks.

A tour of the code in `stile/` as-is, roughly in order of importance/amount of review needed:
- `sys_tests.py` - the main systematics test code file. Right now this has some correlation function code that operates via corr2 (`CorrelationFunctionSysTest` and children), as well as a Stats sys test (`StatSysTest`) that was part of branch #6/PR #15--so that's already been reviewed.
- `corr2_utils.py` - all the things that `CorrelationFunctionSysTest` needs in order to talk to corr2. This includes some validation of the parameters to write to a config file (`CheckArguments`); some functions to write config files for corr2 (`WriteCorr2ConfigurationFile`) and read the results of a run of corr2 (`ReadCorr2OutputFile`); to harvest the corr2-related arguments from a dict containing other parameters as well (`AddCorr2Dict`--see caveats below); and figure out which data sets need to be written to files on disk and make sure they all have the same columns corresponding to ra/dec/g1/g2/etc (`MakeCorr2FileKwargs`), plus a bunch of helper functions and variables to make those things easier.
- `file_io.py` - wrapper functions for reading/writing ASCII and FITS files in various ways.
- `stile_utils.py` - other utility functions. The `Stats` class was already checked in #6/#15, so the other main block of code to look at is the `FormatArray` function, which turns arrays which are not NumPy formatted arrays into formatted arrays (ie things which allow you to ask for `arr['ra']` to get all the RA values as well as `arr[0]` to get all the data for object 0). These are not formally numpy.recarrays, but they're similar.
- `binning.py` - describes some `Bin*` objects which set up a binning scheme and, when called, return a list of `SingleBin` objects, which contain the bounds of the bin, each one of which can be called on a data array to return only the data within the bin.
- `data_handler.py` - this is mostly abstract, but defines the kind of interface we'd want for non-specialized (ie non-LSST/HSC) users of Stile to get their data into the program.
- `__init__.py` - loads stuff into the `stile` namespace.

In addition to those changes, per @HironaoMiyatake's suggestion on #2, everything in `tests/` (including the new stuff) has been ported over to the `unittest` framework. We still need to use the `numpy.testing` functions for arrays--`numpy.array_equal` seems to not work in quite the same way as `numpy.testing.assert_array_equal`--which means that failing tests will produce a combination of Errors (from `numpy.testing`) and Failures (from `unittest`). Still, it seems to have some nice summary functionality that doesn't require external packages. For those who haven't used `unittest` before, you can run all the tests in the directory at once via `python -m unittest discover`, or run the individual test files like normal Python scripts.

This also means all the testing files are going to show up in near-entirety in the changed files pane, for those who want line-by-line commenting

Things I'm explicitly still unsure about:
- Right now `corr2_utils.AddCorr2Dict()` takes a dict of parameters, pulls out the ones that can be used by corr2, and creates a new copy of the input dict with an added `corr2_kwargs` key that contains a dict with only the parameters useful for corr2. Better to have it just return the thing that's in `new_dict['corr2_kwargs']` instead? I can't decide.
- The `SingleBin`-type objects in `binning.py` used to return Boolean masks and now just return the already-indexed original array. (That is, you used to have to `data = data[bin(data)]` and now you just `data = bin(data)`. I can see use cases for both, but I think the second thing is simpler enough (and the gains you might get from `and`-ing the masks rather than masking sequentially small enough) to leave it this way.
- I was pretty much coming up with names for things on the fly, so if it seems obscure/convoluted/just wrong, say something!

Other than that, happy for any thoughts anyone has as well.