Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated plotDMN to work with newest DMM version #80

Merged
merged 10 commits into from
Jul 7, 2023
Merged

Conversation

BananaCancer
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Member

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

Copy link
Contributor

@TuomasBorman TuomasBorman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good!

.github/workflows/check-bioc-devel.yml Outdated Show resolved Hide resolved
R/plotDMN.R Outdated Show resolved Hide resolved
R/plotDMN.R Outdated Show resolved Hide resolved
@antagomir
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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

@BananaCancer
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor Author

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

Is this ok to merge?

@antagomir
Copy link
Member

ok!

@antagomir
Copy link
Member

Merge if ready.

@TuomasBorman TuomasBorman merged commit f8b21b5 into master Jul 7, 2023
1 check passed
@TuomasBorman TuomasBorman deleted the plotDMN-update branch July 7, 2023 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants