refactor getBaselineDivergence/getStepwiseDivergence

[Daenarys8]

Dependent on microbiome/mia#635

[Daenarys8]

Given that the calculation has changed, should we still support the baseline_sample parameter as either a vector or a SummarizedExperiment object?

@TuomasBorman
@antagomir

[TuomasBorman]

If I remember correctly addDivergence supports

• Variable from colData
• Numeric vector denoting reference sample
• Character vector denoting reference samples for each sample
• Character scalar denoting a reference sample

The same should/could be supported in miaTime (perhaps use the same function)? However, to make things simpler, we should prioritize the colData variable in the documentation, otherwise it might become too complex.

I think the only difference between addDivergence and addBaselineDivergence is that the latter adds the time difference?

addStepwiseDivergence is little bit more complex since we should specify the previous step/sample for each sample. But after that, the function is same as addDivergence (apart from adding time difference).

[Daenarys8]

@antagomir

[TuomasBorman]

This function is unnecessarily complex. We should aim to reuse existing functions whenever possible, as there is a lot of functionality here that is implemented only for this function, but it isn’t needed. Specifically, much of what this function does can already be handled by mia::addDivergence.

The only features missing from addDivergence that this function requires are:

a) Adding reference values to colData
b) Adding time difference results to colData

These could be integrated more simply without duplicating functionality.

addBaselineDivergence

• (Optional) Set the first time point as the default reference.
  

1. Calculate divergence values with `addDivergence`.
   

2. Add time difference values to `colData`.
   

addStepwiseDivergence

1. Add reference sample information to colData.
2. Calculate divergence values with addDivergence.
3. Add time difference values to colData.

[Daenarys8]

ndimred is longer used in getStepwiseDivertgence

[TuomasBorman]

In the most recent code, I made some changes to minimize redundant code. The functionality of these functions are now ready, however, the documentation is not updated yet.

Moreover, we need to discuss about deprecation (do we need to deprecate or can we just remove and modify). Especially this is relevant in get* functions as they previously added values to colData but now they return only the values (as we have decided this naming scheme).

Moreover, reducedDim() is not now supported. If we want to support it, it needs to be incorporated to mia::getDivergence(). The idea of these miaTime divergence functions is only to add reference samples (and time difference) to colData, mia::getDivergence() does the calculation.

[TuomasBorman]

I think this PR is now ready. However, there is still one important thing to discuss. We have not deprecated the argument names. Moreover, previously get*Divergence functions returned TreeSE with calculated values in colData.

Now they get* functions return DF and add* return TreeSE. Because we will not remove the get* functions but just modify them to return DF, we cannot deprecate them.

Moreover, deprecating argument name causes extra work.

--> Can these arguments and functions be modified without deprecation? I think (but I do not know) that these functions are not that widely used yet, so we can modify them freely. Moreover, this package is not published yet and this has been more like a draft.

@antagomir

[antagomir]

I agree that this can be changed now without deprecation, not yet in wide use.

[antagomir]

Yes, these can be modified without deprecation.

Is this now ready to merge & close?

[antagomir]

Remember to check & update the vignettes and unit tests as well if necessary