[Phylobase-devl] unification of tree data slots

François Michonneau francois.michonneau at gmail.com
Fri Oct 2 18:01:58 CEST 2009


Hi Jim,

 The changes you propose in ticket #671 sound reasonable to me. We'll
indeed need some testing but I don't see any particular issues with
them.

  I designed formatData and .phylo4Data when the slots were separated,
so if it makes more sense to change the order in which they are called
with a single slots for 'data' then go ahead! I also really like the
idea of reducing the redundancy of the arguments between formatData
and .phylo4Data.

  I'm really swamped and don't have much time to code. I'll be happy to
run the new version on a couple of projects using phylobase functions
though.

  Cheers,
  -- François


On Thu, 2009-10-01 at 15:20 -0700, Jim Regetz wrote:
> Thanks François!
> 
> On a related note, did you see my comments in ticket #671 about tdata 
> changes? It's basically a continuation of our discussion in feature 
> request #571. Just curious if you or anyone else had any thoughts before 
> I commit anything.
> 
> One additional question: The presence of the .phylo4Data and formatData 
> helpers made the changes to tdata MUCH easier than they would have been, 
> thanks! However, one slight complication is that .phylo4Data currently 
> calls formatData on each of its input data frames, using the same "..." 
> arguments for each one. In the revised tdata method, I pass the new data 
> to .phylo4Data along with a subset of the existing slot data, and I 
> *don't* want formatData to be called with the same arguments on both. 
> I've solved the problem by first calling formatData as needed to make 
> everything consistent, then passing them to .phylo4Data. Works fine, but 
> it results in formatData being called a second time (inside .phylo4Data) 
> on data that I know are already formatted properly.
> 
> Anyway, would you mind if I pulled the formatData() calls out of 
> .phylo4Data, and instead leave it up to functions that use .phylo4Data 
> to first call formatData() if needed?
> 
> This would mean phylo4d constructor and addData method that call 
> .phylo4Data first have to call formatData on the inputs, like so:
> 
> td <- formatData(phy, td, "tip", ...)
> nd <- formatData(phy, nd, "internal", ...)
> ad <- formatData(phy, ad, "all", ...)
> .phylo4Data(phy, td, nd, ad, merge.data)
> 
> As indicated above, an additional tweak is that .phylo4Data will only 
> need arguments for the tree, the three data frames, and merge.data. All 
> the other arguments are needed only for formatData.
> 
> Let me know if that didn't make any sense...
> 
> Thanks,
> Jim
> 
> François Michonneau wrote:
> >> As part of this process, I did make some user-level changes. Details are 
> >> below for anyone interested. How should we proceed on determining 
> >> whether to apply these changes to trunk? We also have Peter's NA->0 
> >> changes in the queue, and if we adopt both, we'll need to coordinate our 
> >> merges-to-trunk in a sensible way.
> > 
> >   I haven't really had a chance to test the new functionalities but the
> > changes you introduce look great, so I agree for merging your branch
> > into the trunk.
> > 
> >   I also read over the user-visible changes, and I think the choices you
> > made are all sensible. Thanks for fixing the behavior of merge.data
> > (this was on my TODO list).
> > 
> >   I enforced the size of the data frames in checkPhylo4Data mainly
> > because it's what was expected to be returned by the phylo4d
> > constructor. So, if we would have to use this assumption, then it was a
> > test to make sure that the user doesn't modify the slots. However, as
> > the behavior of the constructor has changed, I think it's OK to remove
> > the code you commented out.
> > 
> >> One somewhat superficial question -- I called the new slot 'data'. Any 
> >> strong feelings on whether it should be 'tdata' instead?
> > 
> > I slightly prefer 'data'.
> > 
> >   -- François
> > 
> >> Cheers,
> >> Jim
> >>
> >> ------------------------------------------------------------------------
> >>
> >> _______________________________________________
> >> Phylobase-devl mailing list
> >> Phylobase-devl at lists.r-forge.r-project.org
> >> https://lists.r-forge.r-project.org/cgi-bin/mailman/listinfo/phylobase-devl
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part
Url : http://lists.r-forge.r-project.org/pipermail/phylobase-devl/attachments/20091002/cf32148d/attachment.pgp 


More information about the Phylobase-devl mailing list