search

microbiome / miaTime

Microbiome time series analysis
https://microbiome.github.io/miaTime/
Artistic License 2.0
5 stars 4 forks source link

refactor getBaselineDivergence/getStepwiseDivergence #91

Closed Daenarys8 closed 2 weeks ago

Daenarys8 commented 2 months ago

Dependent on https://github.com/microbiome/mia/pull/635

Daenarys8 commented 2 months ago

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 commented 2 months ago

If I remember correctly addDivergence supports

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 commented 1 month ago

@antagomir

Daenarys8 commented 1 month ago

ndimred is longer used in getStepwiseDivertgence

TuomasBorman commented 2 weeks ago

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 commented 2 weeks ago

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

antagomir commented 2 weeks ago

Yes, these can be modified without deprecation.

Is this now ready to merge & close?

antagomir commented 2 weeks ago

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