Adding assay_name aliases

alanocallaghan / scater

Clone of the Bioconductor repository for the scater package.

https://bioconductor.org/packages/devel/bioc/html/scater.html

94 stars 40 forks source link

Adding assay_name aliases #187

Closed antagomir closed 1 year ago

antagomir commented 1 year ago

This PR addresses issue #186 .

Changes:

Added assay_name as an alias for exprs_values in function calls ("API"); also changed the functions to use assay_name internally so that deprecation of exprs_values later is easier; no changes in actual functionality have been introduced.
Similar aliases and updates to by_exprs_values (by_assay_name), altexp_exprs_values (altexp_assay_name) and exprs_logged (assay_logged)
Very minor code polishing on the way
DESCRIPTIOn file info updated accordingly
manpages updated with devtools::document()

Checks:

All tests pass, except some UMAP related failure that is independent of this PR.
build(), check(), BiocCheck() OK (with that one independent exception)

TODO (to discuss before adding):

Update inst/NEWS.md if this is desirable?
Add deprecation messages for exprs_values (or perhaps later?)
Update examples, tests, and vignettes to use assay_name (or perhaps later?)

antagomir commented 1 year ago

Ok I fixed the minor issue that caused one test to fail; it was related to assay_name alias; now all tests go through.

I also added conda update GHA .github/workflows/ as the GHA build failure explicitly states: "Please update conda by running $ conda update -n base -c defaults conda"

alanocallaghan commented 1 year ago

I'll review this when I'm back at work in the new year

alanocallaghan commented 1 year ago

Sorry for the delay, can you make those roxygen changes and then rerun devtools::document()?

antagomir commented 1 year ago

In this current PR we have the new argument documented in roxygen as: #' @param assay_name Alias for exprs_values.

Following this request I will now change this the other way round.

The exprs_values is still included (with a deprecation message).

antagomir commented 1 year ago

I have now added that requested update and the checks seemed to pass.

alanocallaghan commented 1 year ago

You need to wrap argument names with \code{} in roxygen, eg #' @param assay_name Alias for \code{exprs_values}.

I didn't request that you change what's an alias for what, although this change is fine

antagomir commented 1 year ago

Yea.. just lazy sometimes :-) I have now added this in all the occurrences that I could identify.

antagomir commented 1 year ago

Just let me know then if there is anything else I could add to the PR.

alanocallaghan commented 1 year ago

There's a couple of pending review comments on the current version above

antagomir commented 1 year ago

There's a couple of pending review comments on the current version above

Thanks! I found one more occurrence for wrapping the \code{} and I have run the devtools::document().

True, I should not have changed what is alias for what; I can change back where necessary but not doing anything for now as it seems it is ok.

I cannot see other comments. I was looking for, the review box on top right says: "No reviews". Just let me know if I have missed any other comments somehow.

alanocallaghan commented 1 year ago

Well I have no idea why you're not seeing the reviews I tagged you in, that's very unintuitive.

Please remove these two edits:

https://github.com/Alanocallaghan/scater/blob/b195432a5b927a6caf56296cd7002f25cf58cf2b/.github/workflows/R-CMD-check.yaml#L21

https://github.com/Alanocallaghan/scater/blob/06d6977195cdd37b268e4336a5af4ad557f604bc/R/plotReducedDim.R#L209-L210

alanocallaghan commented 1 year ago

Thanks!

antagomir commented 1 year ago

Yes, indeed strange. I never had problems in seeing PR reviews before. Happy to add more fixes as needed :-)