[Bioperl-l] return type of $feature->seq() (comments on a commit [bioperl/bioperl-live fcd90e0])

[Chris Fields]

I agree, but we spotted this from IRC, then added the comments on that merge.  Dave also spotted my original code comments (which appeared in the fork queue, and which echo the very same concerns you have) after the commit as well, and managed to revert it.  So, with forked where it appears further discussion is warranted (like this), we should bring it to the main list (and IRC, if anyone happens to be there) for discussion.  Sounds good to me.

For those on list, here are Adam's and my comments on this (linked here: http://github.com/adsj/bioperl-live/commit/24ec961b217084e248f4fdbd174aadace1a27ac4#comments):

adsj:

"Hi Chris, thanks for the comment. The reason is this: I have a class, MyApp::Seq, which ISA Bio::Seq::RichSeq and adds some extra methods I use in the application. When I call ->seq() on a feature from one of my MyApp::Seq objects, I want to get a MyApp::Seq object back (because of the extra methods). Am I making sense? I have been running with this patch since at least 1.5.2, so it has been a while since I digged into it. Maybe there is a cleaner solution. I am not sure what your comment about changing the API means - I think it is quite reasonable/natural that MyApp::Seq->get_Features"->seq" returns MyApp::Seq objects?"

My response:

"Calling seq() on a feature should return a truncation of whatever your Bio::SeqFeatureI does (it normally calls trunc(start, end) on it's attached sequence). For Bio::Seq it's normally returning a simple Bio::PrimarySeq, not a Bio::Seq, b/c that is what is attached to the Feature. This is why we don't need GC. There are no circular refs: Bio::Seq has-a PrimarySeq and has-a Features (via FeatureHolderI), each Feature has the same PrimarySeq as the parent Bio::Seq.

It's hard to know if there is a workaround w/o knowing what you are asking for (e.g. what MyApp::Seq does), but you can certainly override the default methods to DTRT for your specific case. For instance, redefine add_SeqFeature() for your class to attach self as you have above for Bio::Seq. In this case, we should patch SeqFeature::Generic to use weaken() as you show above just in case this is needed by others, but maybe in the context of (pseudocode) 'weaken if $seq to be attached is-a Bio::SeqI', and not hammered down to check the very specific 'Bio::PrimarySeq'.

Anyway, this is what I mean by changing the default API, which is what the above Bio::Seq change does. This would change the context of what is currently being returned (self, instead of a simpler contained Bio::PrimarySeqI). Also, anything gained by abstracting the raw seq handling of Feature data by linking to PrimarySeq is lost when you link to the parent, thus always requiring GC and weaken() (which is notoriously flaky dep. on context)."

chris

On May 25, 2010, at 4:10 PM, Hilmar Lapp wrote:

> I'm a little concerned that this discussion is disconnected from the list and so misses a lot of possible input. Are we moving our development discussion to IRC or github commit comments?
> 
> Regarding $feature->seq(), the API documentation expressly states that the return type is Bio::PrimarySeqI, as it does for $feature->entire_seq().
> 
> The original rationale for that was to avoid circular references. Bio::SeqI objects contain references to attached features, which in turn contain a reference to the seq object they are attached to.  A Bio::SeqI object holds the basic sequence properties (everything except annotation and feature objects) in a Bio::PrimarySeq delegate, which is what a feature keeps a reference to, not the containing Bio::SeqI object.
> 
> It's possible that S::U::weaken() can solve the circular reference problem, but this fact should be tested. I.e., attach a feature with a SeqI-reference to a SeqI, dispose the SeqI, and then test that the feature has lost the reference to the SeqI too.
> 
> This still leaves the issue though that then you have a SeqFeatureI object with a dangling reference to a sequence object. If you have those SeqFeatureI objects stored in a feature store, this may wreak havoc. I'd like to see convincing arguments that it doesn't.
> 
> Bottom line - just forking on git and committing a change isn't a substitute for bringing up an issue and possible solutions on the list, and the vetting of pull requests can fall upon only one or two core developers. Two eyeballs often spot a lot less than a hundred.
> 
> 	-hilmar
> 
> On May 25, 2010, at 2:02 PM, GitHub wrote:
> 
>> Ah, but my link's old, forget it.  This one is better: http://book.git-scm.com/4_undoing_in_git_-_reset,_checkout_and_revert.html
>> 
>> From: cjfields
>> View this commit online: http://github.com/bioperl/bioperl-live/commit/fcd90e0f2fa94b61ff8351157129678417c32991
> 
> -- 
> ===========================================================
> : Hilmar Lapp  -:-  Durham, NC  -:-  hlapp at gmx dot net :
> ===========================================================
> 
> 
> 
> 
> 
> _______________________________________________
> Bioperl-l mailing list
> Bioperl-l at lists.open-bio.org
> http://lists.open-bio.org/mailman/listinfo/bioperl-l