[Insight-developers] Change in ITK[master]: WIP: Accumulated changes to Transform and effected classes.

Sun Aug 14 17:27:26 EDT 2011

Hi,

I think one JIRA issue may have been missed.  Based on my mental math
(which could certainly be flawed) i was not convinced the GetJacobian
method in the CompositeTransform was working as it should.  Mainly
because i dont see how it can, without calling the
GetJacobianWithRespectToPosition, but it doesnt call it.  In any case,
Michael asked me to treat this as work in progress and not do a full
numerical test yet.  It should be done someday though, and i am
notoriously forgetful.   I'm not sure how to create a JIRA issue or
even if i can, but we should be sure not to forget this one.

The registration refactoring commit
http://review.source.kitware.com/2096 is also a huge patch set that
changes a lot of stuff.  It risks to suffer from the same problem.  If
theres any way at all to split it up into pieces that might make the
reviews go faster.  Ideally, I would like to be sure i understand the
design intent first - then i can check that the code actually delivers
what the design intends that it should.

I feel i should point out that what makes these so tough to review is
there is a massive amount of very good, detailed work involved.   To
give it the thorough review it deserves is inevitably time consuming,
I hope those actually doing the work arent getting discouraged by all
the questioning.

Cheers,
Rupert
--------------------------------------------------------------
Rupert Brooks
rupert.brooks at gmail.com

On Fri, Aug 12, 2011 at 10:45, Matt McCormick
<matt.mccormick at kitware.com> wrote:
> Hi Bill,
>
> The API is still changing.  In particular, the TransformParameters
> class will likely be renamed to OptimizerParameters.
>
> There are a number of important remaining issues to address, for which
> I will file JIRA tickets.  These are noted my myself and Michael in
> the ending Gerrit comments.  If anything was missed, please file
> another ticket.
>
> This was merged to keep the code flowing into the alpha (while it is
> still alpha).  This patch was originally submitted July 15th, there
> were few reviews, and the improvements to the code did not progress in
> an efficient manner.  Big patch sets that accomplish many things do
> not progress very quickly.
>
> Best,
> Matt
>
> On Fri, Aug 12, 2011 at 8:33 AM, Bill Lorensen <bill.lorensen at gmail.com> wrote:
>> I wish a few more reviewers had looked at this large change before it was
>> merged. I had not planned on reviewing it until it changed from WIP to ENH.
>> That is my bad however.
>>
>> Let's clean up the warnings on this one quickly. We'll have a better feel
>> for the extent of the warnings tomorrow. Also we can see if there are any
>> valgrind issues.
>>
>> I did not see a migration document. Does this mean that the API has not
>> changed? If so, that is fantastic.
>>
>> Bill
>>
>> On Fri, Aug 12, 2011 at 8:31 AM, Bill Lorensen <bill.lorensen at gmail.com>
>> wrote:
>>>
>>> I wish a few more reviewers had looked at this large change before it was
>>> merged. I had not planned on reviewing it until it changed from WIP to ENH.
>>> That is my bad however.
>>>
>>> Let's clean up the warnings on this one quickly. We'll have a better feel
>>> for the extent of the warnings tomorrow. Also we can see if there are any
>>> valgrind issues.
>>>
>>> I did not see a migration document. Does this mean that the API has not
>>> changed? If so, that is fantastic.
>>>
>>> Bill
>>>
>>> On Fri, Aug 12, 2011 at 7:44 AM, Matt McCormick
>>> <matt.mccormick at kitware.com> wrote:
>>>>
>>>> Sorry, Bill, I missed your comment.
>>>>
>>>> Matt
>>>>
>>>> On Thu, Aug 11, 2011 at 7:21 PM, Bill Lorensen <bill.lorensen at gmail.com>
>>>> wrote:
>>>> > I'd like to try this patch against ITKApps and Slicer4 before it is
>>>> > merged.
>>>> >
>>>> > I hope to try it this weekend.
>>>> >
>>>> > Bill
>>>> >
>>>> > On Thu, Aug 11, 2011 at 6:36 PM, Code Review <review at kitware.com>
>>>> > wrote:
>>>> >>
>>>> >> From Matt McCormick <matt.mccormick at kitware.com>:
>>>> >>
>>>> >> Matt McCormick has posted comments on this change.
>>>> >>
>>>> >> Change subject: WIP: Accumulated changes to Transform and effected
>>>> >> classes.
>>>> >> ......................................................................
>>>> >>
>>>> >>
>>>> >> Patch Set 4:
>>>> >>
>>>> >> Great work, Michael.  I will merge after the builds go through.
>>>> >>
>>>> >> Have a good weekend.
>>>> >>
>>>> >> --
>>>> >> To view, visit http://review.source.kitware.com/2087
>>>> >> To unsubscribe, visit http://review.source.kitware.com/settings
>>>> >>
>>>> >> Gerrit-MessageType: comment
>>>> >> Gerrit-Change-Id: Ie806bb1897549403c11281e4921ea8025edba2b4
>>>> >> Gerrit-PatchSet: 4
>>>> >> Gerrit-Project: ITK
>>>> >> Gerrit-Branch: master
>>>> >> Gerrit-Owner: Michael Stauffer <mstauff at verizon.net>
>>>> >> Gerrit-Reviewer: Andinet Enquobahrie <andinet.enqu at kitware.com>
>>>> >> Gerrit-Reviewer: Bill Lorensen <bill.lorensen at gmail.com>
>>>> >> Gerrit-Reviewer: Jeffrey Duda <jeff.duda at gmail.com>
>>>> >> Gerrit-Reviewer: Jim Miller <millerjv at ge.com>
>>>> >> Gerrit-Reviewer: Luis Ibanez <luis.ibanez at kitware.com>
>>>> >> Gerrit-Reviewer: Marius Staring <m.staring at lumc.nl>
>>>> >> Gerrit-Reviewer: Matt McCormick <matt.mccormick at kitware.com>
>>>> >> Gerrit-Reviewer: Michael Stauffer <mstauff at verizon.net>
>>>> >> Gerrit-Reviewer: Nick Tustison <ntustison at gmail.com>
>>>> >> Gerrit-Reviewer: Rupert Brooks <rupert.brooks at gmail.com>
>>>> >> Gerrit-Reviewer: brian avants <stnava at gmail.com>
>>>> >
>>>> >
>>>
>>
>>
>> _______________________________________________
>> Powered by www.kitware.com
>>
>> Visit other Kitware open-source projects at
>> http://www.kitware.com/opensource/opensource.html
>>
>> Kitware offers ITK Training Courses, for more information visit:
>> http://kitware.com/products/protraining.html
>>
>> Please keep messages on-topic and check the ITK FAQ at:
>> http://www.itk.org/Wiki/ITK_FAQ
>>
>> Follow this link to subscribe/unsubscribe:
>> http://www.itk.org/mailman/listinfo/insight-developers
>>
>>
> _______________________________________________
> Powered by www.kitware.com
>
> Visit other Kitware open-source projects at
> http://www.kitware.com/opensource/opensource.html
>
> Kitware offers ITK Training Courses, for more information visit:
> http://kitware.com/products/protraining.html
>
> Please keep messages on-topic and check the ITK FAQ at:
> http://www.itk.org/Wiki/ITK_FAQ
>
> Follow this link to subscribe/unsubscribe:
> http://www.itk.org/mailman/listinfo/insight-developers
>