[Insight-developers] Re: itkSubsample issues

[Brad Davis]

I agree that it comes down to a backwards compatibility issue.
However, I think there is strong argument for making the change.

First, I would argue that the current implementation is a bug.  It is
completely unintuitive: as in your example, why should the sorted
subsample not be sorted?  Why should it contain vectors that have not
been added to the subsample.  Also, for example, it is perfectly legal
to create a subsample of a subsample, but if you use the
itkMeanCalculator on this sub-subsample you will get surprising and
meaningless results---there is no warning of this behavior.

Second, I think there is a huge benefit to allowing code to treat
samples and subsamples in the same way.  For example, I wrote a class
that generates random subsample from a sample.  That class will not
work on the GetClassSample output of a MembershipFunction because that
output is a subsample (this would create a subsample of a subsample
which is legal but does not work, as implemented, in a meaningful
way).

Even if the GetMeasurementVector method can't be change for backwards
compatibility issues, I think that it would be benificial to create a
common and intuitive interface for samples and subsamples.  As I have
said, this would allow more flexible code as well as nested
subsamples.

Brad

On 8/15/06, Karthik Krishnan <Karthik.Krishnan at kitware.com> wrote:
> Hi Brad,
>
> IMHO, as you mentioned there is a BCP issue to address here. (While
> there may be no objection here, they'll come in the form of mails in the
> users list later.. we've seen this once before :) ).
>
> For instance if somebody uses a subsample and then sorts it, for
> querying the quantile, and then queries samples as
>   subsample->GetMeasurementVector(1),
>   subsample->GetMeasurementVector(2)
>    .....
> will have the unsorted sample instances and with the proposed change
> will have the sorted sample instances. This may not be an uncommon use case.
>
> And while your proposed change is the reasonable behaviour, it will
> break code. The scary thing is that its not something that will generate
> a compiler error, (which I personally think is ok, despite the BCP). It
> quietly changes the runtime behaviour.
>
> Can you not make do with the subsample's iterator API ? or the
> GetMeasurementVectorByIndex method, (which I admit forces you to be
> aware of the fact that its a subsample).
>
> my 2 cents.
> -karthik
>
> Brad Davis wrote:
>
> > By the way, I did run ctest (cvs version of Insight from yesterday)
> > and all the tests passed with this change in place (linux x86_64, gcc
> > 4.0.4).
>
> >
> > On 8/15/06, Brad Davis <brad.davis at kitware.com> wrote:
> >
> >> I propose changing the implementation of GetMeasurementVector in the
> >> Subsample class from
> >>
> >> const MeasurementVectorType & GetMeasurementVector(const
> >> InstanceIdentifier &id) const
> >> { return m_Sample->GetMeasurementVector(id) ; }
> >>
> >> to
> >>
> >> const MeasurementVectorType & GetMeasurementVector(const
> >> InstanceIdentifier &id) const
> >> { return m_Sample->GetMeasurementVector(m_IdHolder[id]) ; }
> >>
> >> First, the current implementation allows access to measurement vectors
> >> that are not in the subsample---GetMeasurementVector gets vectors from
> >> the sample, not the subsample.  Second, while the
> >> GetMeasurementVectorByIndex() method can be used to return the
> >> measurement vectors that are actually in the sample, the change in
> >> interface means that it is difficult to write general code that will
> >> work for a sample or a subsample.  Also, GetMeasurementVectorByIndex
> >> returs by value rather than by reference.  Iterators can be used but
> >> using both iterators and GetMeasurementVectorByIndex() it is
> >> impossible to have a working subsample of a subsample---the
> >> implementation only allows for one level of indirection.  Again this
> >> means that you have to know a priori if you are working with a sample
> >> or subsample.
> >>
> >> The change that I propose allows Samples and Subsamples to be
> >> addressed by a common interface, and allows the programmer to create a
> >> Subsample of either a Sample or a Subsample.  This would increase the
> >> flexibility of the classes and allow for more general code.
> >>
> >> The drawback of the change is that if someone is already using the
> >> GetMeasurementVector() method of the Subsample class to directly (and
> >> incorrectly, I argue) access vectors from the Sample, then the change
> >> is not backward compatible.  However, access to the Sample can also be
> >> gained by Subsample's GetSample method.
> >>
> >> The other mechanism for fixing this problem is to add a
> >> GetMeasurementVectorByIndex() method to the Sample class, and change
> >> the method to return by reference.  This does not require changing the
> >> GetMeasurementVector method but it makes the classes more confusing.
> >>
> >> Brad
> >>
> > _______________________________________________
> > Insight-developers mailing list
> > Insight-developers at itk.org
> > http://www.itk.org/mailman/listinfo/insight-developers
> >
>