microbiome / miaViz

Microbiome Analysis Plotting and Visualization
https://microbiome.github.io/miaViz
Artistic License 2.0
10 stars 12 forks source link

Updated plotDMN to work with newest DMM version #80

Closed BananaCancer closed 1 year ago

BananaCancer commented 1 year ago

With the recent/upcoming modifications to DMM in mia and the addition of the algorithm to bluster, this function needs to change to work with the newest version of DMM. I changed it so that it still works as before by putting a warning if the DMM info is in the wrong place. It works as intended if the tse contains the info in the old place (metadata$DMM) or new place (metadata$DMM$dmm)

BananaCancer commented 1 year ago

@antagomir @TuomasBorman Is this ok for you? I also added bluster in the workflow script and added bluster in suggests in the DESCRIPTION file. The NEWS and version have also been updated.

antagomir commented 1 year ago

Seems good to me. Also check if this is something to show in OMA.

antagomir commented 1 year ago

We could also consider if the DMM slot there needs to be readily calculated. I don't think this is used very much in our examples?

BananaCancer commented 1 year ago

We could also consider if the DMM slot there needs to be readily calculated. I don't think this is used very much in our examples?

So for example, if neither metadata(tse)$DMM$dmm and metadata(tse)$DMM exist, we execute a basic DMM call. So this example,

tse <- plotDMNFit(tse, name = "DMM", type = "laplace")

would execute

        if (!is.null(metadata(x)[[name]]$dmm)) {
            dmn <- metadata(x)[[name]]$dmm
        } else if (!is.null(metadata(x)[[name]]) {
            .Deprecated(old="getDMN", new="cluster", 
                    "Now runDMN and calculateDMN are deprecated. Use cluster with DMMParam parameter and full parameter set as true instead.")
            dmn <- metadata(x)[[name]]
        } else {
            tse <- cluster(tse, name = name, DmmParam(k = 1:3, type = "laplace")
            dmn <- metadata(x)[[name]]$dmm
        }
        # ...
        tse
antagomir commented 1 year ago

I was wondering what is the point of having data object that already includes something that users usually need to calculate themselves anyway?

BananaCancer commented 1 year ago

Oh ok, yeah I think it's pretty rare that it's the case. I can add the bluster dependency and change the example to start with a simple SE, add the DMM, and plot the fit.

TuomasBorman commented 1 year ago

I was wondering what is the point of having data object that already includes something that users usually need to calculate themselves anyway?

DMM can take a long time to calculate, that was the only reason

antagomir commented 1 year ago

I was wondering what is the point of having data object that already includes something that users usually need to calculate themselves anyway?

DMM can take a long time to calculate, that was the only reason

Now it seems there was also a second reason: to avoid bluster dependency in miaViz

antagomir commented 1 year ago

I am not sure if it really is a problem to have bluster as a dependency in miaViz. The example might be more clear if the clustering was also done as part of it. If the example uses a small data set with a small maximum number of clusters then perhaps the calculation does not take too long.

TuomasBorman commented 1 year ago

I am not sure if it really is a problem to have bluster as a dependency in miaViz. The example might be more clear if the clustering was also done as part of it. If the example uses a small data set with a small maximum number of clusters then perhaps the calculation does not take too long.

Yep, the dependency issue is not big --> but why have bluster as dependency when we already have mia as dependency (which imports bluster)

BananaCancer commented 1 year ago

After testing, the check fails if bluster isn't in the Suggests, so I'll leave it there.

Is this ok to merge?

antagomir commented 1 year ago

ok!

antagomir commented 1 year ago

Merge if ready.