[Bioperl-l] Bio::FeatureHolderI interface confusion

[Chris Mungall]

On Wed, 18 Jun 2003, Lincoln Stein wrote:

> On Wednesday 18 June 2003 04:22 pm, Chris Mungall wrote:
> > On Wed, 18 Jun 2003, Lincoln Stein wrote:
> > > To answer Chris's question, the current interface would require you to:
> > >
> > > 	1) open a Bio::SeqFeatureIO
> > > 	2) fetch a Bio::SeqI from it
> > > 	3) call Bio::SeqI->get_SeqFeatures to get one or more Bio::SeqFeatureI's
> > > 	4) for each Bio::SeqFeatureI, you can rely on them to respond
> > > 		to the sub_SeqFeatures() call, although they usually will
> > > 		usually return an empty list
> > >
> > > Maybe I'm missing the point of the discussion, but there's no violation
> > > of the contract here, and this is what Chris is asking for.  No need to
> > > call isa() or can().
> >
> > Step 4 is a violation; where in the Bio::SeqFeatureI code or documentation
> > does it state that you can use the method sub_SeqFeatures()?
>
> My error.  The relevant Bio::SeqFeatureI method name is get_SeqFeatures() --
> same as Bio::SeqI.  I think this is a bug in FeatureHolderI too, where the
> POD docs refer to sub_SeqFeatures() and the code refers to get_SeqFeatures().
>
> Have we been arguing about a typo?

possibly. My brain is melting down at this point.

My actual use case was for adding sub seq features

I call:

  $sf->add_SeqFeature(...)

Which is in the pod docs for SeqFeature::Generic, and I don't think it is
in any interface. So the point remains, SeqFeature::Generic is a
completely hidden implementation class, yet I am violating the SeqFeatureI
contract by calling add_SeqFeature on it.

Maybe this is deliberate? Maybe features are meant to be immutable with
respect to nesting?

> > You said: "you can rely on them to respond to the sub_SeqFeatures() call"
> >
> > Yes, I can rely on them to do this purely because I cheated and I ignored
> > the contract and saw that the implementing class is
> > Bio::SeqFeature::Generic
>
> Oy vey.  As far as I can see, this is all the propagation of a POD typo.
> Look here at the top of the FeatureHolderI file:
>
>   Popular feature-holders are for instance L<Bio::Seq> objects. Since
>   L<Bio::SeqFeatureI> defines a sub_SeqFeature() method, most
>   Bio::SeqFeatureI implementations like L<Bio::SeqFeature::Generic> will
>   implement the feature holder interface as well.
>
> This isn't true; it's called get_SeqFeatures() in Bio::SeqFeatureI.
>
> Then below (still in FeatureHolderI):
>
>  =head2 get_SeqFeatures
>
>   Title   : get_SeqFeatures
>   Usage   :
>   Function: Get the feature objects held by this feature holder.
>   Example :
>   Returns : an array of Bio::SeqFeatureI implementing objects
>   Args    : none
>
>  At some day we may want to expand this method to allow for a feature
>  filter to be passed in.
>
>  =cut
>
>  sub get_SeqFeatures{
>      shift->throw_not_implemented();
>  }
>
> Clearly the right brain didn't know what the left was doing.  Then in
> Bio::Seqfeature::Generic:
>
>   # we revamped the feature containing property to implementing
>   # Bio::FeatureHolderI
>   *sub_SeqFeature = \&get_SeqFeatures;
>   *add_sub_SeqFeature = \&add_SeqFeature;
>
> > Step 4 would not be a contract violation if Bio::SeqFeatureI inherited
> > from Bio::FeatureHolderI - it does not.
> >
> > Yes, I can do a test:
> >
> >   if ($sf->isa("Bio::FeatureHolderI")) {
> >     $sf->add_SeqFeature($foo);
> >   }
> >   else {
> >     $self->throw("feature does not have ability to hold subfeatures");
> >   }
>
> This is yuck.  Bio::SeqI implements get_SeqFeatures().  Always has, always
> will.

you mean "Bio::SeqFeatureI implements get_SeqFeatures()" I think; but
point taken. I have definitely been confused by something.

the variety of method names for one thing  eg - sub_SeqFeature

the fact that the same signature seems to be shared amongst multiple
interfaces

FeatureHolderI

	get_SeqFeatures
	get_all_SeqFeatures

SeqI   (which ISA FeatureHolderI)

	get_SeqFeatures
	get_all_SeqFeatures

(are these copied and pasted from FeatureHolderI or are they the actual
interface?)

SeqFeatureI

	get_SeqFeatures
	(but no get_all_Seqfeatures)

And nothing for modifying the list of 'sub' SeqFeatures

I was also confusing myself by looking at:

CollectionI

	add_features
	features

So I retract, the contracts are not wrong. It's just it drives me insane
trying to read them.

There is still the one outstanding question of adding seq features

> sub_SeqFeature() must die!
>
> Lincoln
>
>