[Phylobase-devl] unification of tree data slots

[Jim Regetz]

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