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
Reviewer Review Type Date Requested Status
Matthew Brett Disapprove
Review via email: mp+3816@code.qastaging.launchpad.net
To post a comment you must log in.
Revision history for this message
Fernando Perez (fdo.perez) wrote :

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.

Revision history for this message
Matthew Brett (matthew-brett) wrote :

A small request for status on this branch

review: Needs Information
Revision history for this message
joep (josef-pktd) wrote :

> A small request for status on this branch

lp:~nipy-developers/nipy/trunk-josef-models formed the starting point for the gsoc rewrite, and is therefore superseded by it.

Additionally, nipy/trunk has a different rewrite of stats.models in revision 1709, which seems to make this branch redundant or incompatible.

Revision history for this message
Matthew Brett (matthew-brett) wrote :

It sounds like this one should be decommissioned; I'll Disapprove this merge unless I hear from someone in 24 hours or so.

review: Needs Information
Revision history for this message
Matthew Brett (matthew-brett) wrote :

Disapproved on basis that this branch is now outdated

review: Disapprove

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

Subscribers

People subscribed via source and target branches