Merge lp://qastaging/~nipy-developers/nipy/trunk-josef-models into lp://qastaging/~nipy-developers/nipy/obsolete-do-not-use
Proposed by
Fernando Perez
Status: | Rejected |
---|---|
Rejected by: | Matthew Brett |
Proposed branch: | lp://qastaging/~nipy-developers/nipy/trunk-josef-models |
Merge into: | lp://qastaging/~nipy-developers/nipy/obsolete-do-not-use |
To merge this branch: | bzr merge lp://qastaging/~nipy-developers/nipy/trunk-josef-models |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Matthew Brett | Disapprove | ||
Review via email:
|
To post a comment you must log in.
Unmerged revisions
- 1699. By Fernando Perez
-
Merge changes made with J. Taylor during the stats code review.
- 1698. By Fernando Perez
-
Merge with upstream trunk
This branch includes Josef's original work, plus changes made by J. Taylor and F. Perez on review. Below I'm pasting verbatim the notes we took during our review, since there is still some work left to be done. By making this a nipy team branch, it should be easy to quickly finish implementing any remaining changes and merging this into trunk.
Apologies for some of the short comments in the text below, these were notes taken during a work session.
======= ======= ======= ======= ======= ===== ======= ======= ======= ======= =====
Review comments on josef-models branch
=======
- Branch should be made as a proper nipy project branch, so it can be tracked
by launchpad and used in review.
- The .initialize() in models.glm.Link replaced by a call to the link object
itself. It was OK to call initialize in the first place, the bug was in that
initialize was NOT calling the link object. So the proper fix should be to
instead change the definition of initialize to::
def initialize(...):
return OLD CODE # remove this
return self( OLD CODE) # put this instead
..
This will use the actual link transform on the array previously computed by
initialize, thus returning the sample mean transformed by the proper link
function.
- The yule_walker change to a standalone function is OK. Before committing
the code, the commented-out parts should be removed.
- class Huber -> Huber(object). This is good, but it might as well be done
everywhere. Today ALL code should use new-style classes, so we might as well
clean things up and make them new-style everywhere. Things like the base
Link class, for example, are candidates for conversion.
- Line 204, test_formula, can just be deleted. c.matrix will be accessed
anyway in the next line, so there's no need
- test_ols: use np.test asserts instead of naked asserts. They have better
diagnostics and remain even if running under -O (which plain assert
statements do not).
- test_ols: a test function was renamed to check_xxx but it isn't used
anywhere. Its renaming is OK, but there should be actual tests using it.
- test_ols: the failing test at the end can be moved back into the other one,
so the rlm is available. Using a yield will still give us separate test
results, without having to recompute anything unnecessarily.
- test_armodel: good fix, we'll just rename the counters to be a bit more
explicit on what they do (order/iteration).
- Huber: we'll clean up the code in there, it's a bit messy.
- Note: some of these changes have *already* been applied by J. Taylor in the
nipy trunk. So please make sure that you do a full merge of trunk with your
branch before proceeding further. They were all done in a single revision,
here: so you can easily see what was done.