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

resolve conflicts for dplyr/tidyr funcs #287

Closed
wants to merge 5 commits into from
Closed

resolve conflicts for dplyr/tidyr funcs #287

wants to merge 5 commits into from

Conversation

chilampoon
Copy link

Rewrote Roxygen docs for dplyr_methods.R and tidyr_methods.R with the same (updated) format in tidySCE & tidyseurat.

Now the conflicts when loading tidyomics become

> library(tidyomics)
── Attaching core tidyomics packages ────────────────────────────────────────────────────────── tidyomics 0.1.1 ──
✔ dplyr                    1.1.2tidyr                    1.3.0ggplot2                  3.4.3tidyseurat               0.7.2nullranges               1.6.2tidySingleCellExperiment 1.11.6plyranges                1.19.0tidySummarizedExperiment 1.10.0tidybulk                 1.11.5     
── Conflicts ──────────────────────────────────────────────────────────────────────────── tidyomics_conflicts() ──
✖ plyranges::between()               masks dplyr::between()
✖ ttservice::bind_cols()             masks tidySummarizedExperiment::bind_cols(), dplyr::bind_cols()
✖ ttservice::bind_rows()             masks tidySummarizedExperiment::bind_rows(), dplyr::bind_rows()
✖ tidySummarizedExperiment::count()  masks matrixStats::count(), dplyr::count()
✖ plyranges::filter()                masks tidySummarizedExperiment::filter(), dplyr::filter(), stats::filter()
✖ plyranges::n()                     masks dplyr::n()
✖ plyranges::n_distinct()            masks dplyr::n_distinct()
✖ tidySummarizedExperiment::rename() masks S4Vectors::rename(), dplyr::rename()
✖ plyranges::slice()                 masks tidySummarizedExperiment::slice(), IRanges::slice(), dplyr::slice()
ℹ Use the conflicted package to force all conflicts to become errors

@stemangiola do you want me to update ttservice & tidySE as well..?

@stemangiola stemangiola changed the title resolve conflicts for dlpyr/tidyr funcs resolve conflicts for dplyr/tidyr funcs Aug 27, 2023
@stemangiola
Copy link
Owner

thanks @chilampoon ! some tests are failing.

Feel free to fix 'ttservice' as well.

@chilampoon
Copy link
Author

chilampoon commented Aug 28, 2023

Locally no error now

0 errors| 1 warning| 5 notesError: R CMD check found WARNINGs
Execution halted

I guess others are working on these warnings & notes? 👀

For the deseq2 loading I end up adding those detect & install lines otherwise here

test_that("DESeq2 differential trancript abundance - no object",{

	res_deseq2 =
		test_deseq2_df |>
		DESeq2::DESeq() |>
		DESeq2::results()

DESeq2:: causes errors

R/zzz.R Outdated Show resolved Hide resolved
@stemangiola
Copy link
Owner

@chilampoon please have a look to the failed checks.

@chilampoon
Copy link
Author

It's 0 errors and 0 warnings on my local machine now, I don't know why there was a warning Warning: Warning: declared S3 method 'filter.tidybulk' not found in github checks...

@stemangiola
Copy link
Owner

It's 0 errors and 0 warnings on my local machine now, I don't know why there was a warning Warning: Warning: declared S3 method 'filter.tidybulk' not found in github checks...

Interesting, @HelenaLC any idea about this? @chilampoon did you follow 100% the tidySCE example. The solution must be in some difference between the two.

@chilampoon
Copy link
Author

It's 0 errors and 0 warnings on my local machine now, I don't know why there was a warning Warning: Warning: declared S3 method 'filter.tidybulk' not found in github checks...

Interesting, @HelenaLC any idea about this? @chilampoon did you follow 100% the tidySCE example. The solution must be in some difference between the two.

Currently the tests are failing because of dependencies, some packages are required to install to finish R CMC CHECK. I think it's better to revise and modify all documentations in the package. I can do it if this task hasn't been assigned. Also if @HelenaLC could review a little that would be helpful, thanks!

@stemangiola
Copy link
Owner

stemangiola commented Aug 29, 2023

Currently the tests are failing because of dependencies, some packages are required to install to finish R CMC CHECK.

Apologies, I tried a newer .github action and screwed the CHECKS, I reverted back.

Please pull the new master, which now has the old .github workflow, which fails only for linux, but works for the others.

Comment on lines -854 to +860
library(DESeq2)
library(stringr)

if (find.package("DESeq2", quiet = TRUE) %>% length %>% equals(0)) {
if (!requireNamespace("BiocManager", quietly = TRUE))
install.packages("BiocManager", repos = "https://cloud.r-project.org")
BiocManager::install("DESeq2", ask = FALSE)
}
Copy link
Owner

@stemangiola stemangiola Aug 29, 2023

Choose a reason for hiding this comment

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

I will add this to my PR, so we keep

  • drop DESeq2 dependency, and
  • resolve conflicts

independent.

@stemangiola
Copy link
Owner

I forgot at what stage we are with this

@chilampoon
Copy link
Author

I forgot at what stage we are with this

hi @stemangiola I wonder if anyone is working on the doc/description refactorization or something for tidybulk, if not then I'll probably do it

@stemangiola
Copy link
Owner

(

can you please provide this info for the authorship, if you haven't done already?

<style type="text/css"></style>

Email Institution First Name Middle Name(s)/Initial(s) Last Name Suffix Corresponding Author Home Page URL Collaborative Group/Consortium ORCiD

)

@stemangiola
Copy link
Owner

I forgot at what stage we are with this

hi @stemangiola I wonder if anyone is working on the doc/description refactorization or something for tidybulk, if not then I'll probably do it

Great, let's first fix the github actions here, and then we merge this PR. We can define the next step after that. I believe the documentation has not been updated yet. Could you please inquire about the existing dedicated issue?

@chilampoon
Copy link
Author

the issue is still the filter.tidybulk not found warning, which causes the check failure

❯ checking S3 generic/method consistency ... WARNING
Warning:   Warning: declared S3 method 'filter.tidybulk' not found
  See section ‘Generic functions and methods’ in the ‘Writing R
  Extensions’ manual.

❯ checking dependencies in R code ... NOTE
  Namespace in Imports field not imported from: ‘pkgconfig’
    All declared Imports should be used.
  Unexported objects imported by ':::' calls:
    ‘glmmSeq:::glmmTMBcore’ ‘glmmSeq:::hyp_matrix’ ‘glmmSeq:::lmer_wald’
    ‘glmmSeq:::organiseStats’
    See the note in ?`:::` about the use of this operator.

I don't have this warning locally, I guess it's because of some dependencies problems, plus there are tons of other notes/problems in the report, so I thought it's better to check other scripts as well (not just the dplyr/tidyr functions.R) to resolve the package conflicts..

@stemangiola
Copy link
Owner

stemangiola commented Sep 11, 2023

the issue is still the filter.tidybulk not found warning, which causes the check failure

❯ checking S3 generic/method consistency ... WARNING
Warning:   Warning: declared S3 method 'filter.tidybulk' not found
  See section ‘Generic functions and methods’ in the ‘Writing R
  Extensions’ manual.

❯ checking dependencies in R code ... NOTE
  Namespace in Imports field not imported from: ‘pkgconfig’
    All declared Imports should be used.
  Unexported objects imported by ':::' calls:
    ‘glmmSeq:::glmmTMBcore’ ‘glmmSeq:::hyp_matrix’ ‘glmmSeq:::lmer_wald’
    ‘glmmSeq:::organiseStats’
    See the note in ?`:::` about the use of this operator.

I don't have this warning locally, I guess it's because of some dependencies problems, plus there are tons of other notes/problems in the report, so I thought it's better to check other scripts as well (not just the dplyr/tidyr functions.R) to resolve the package conflicts..

I see, so your strategy would be to also integrate the refactoring of a larger part of the documentation, with the hope that this warning would go away.

I think it is worth it, although I am pretty sure this warning is due to an isolated cause. Did you double-check that your documentation for filter mirrors is 100% the one from SingleCellExperiment?

@chilampoon
Copy link
Author

yes will do it step by step, also most of the SingleCellExperiment documents were refactored

@stemangiola
Copy link
Owner

yes will do it step by step, also most of the SingleCellExperiment documents were refactored

Great, please inform the others on your plans here

#282

we don't want to duplicate efforts. If the other have not started yet (re they are unresponsive, feel free to take action).

@chilampoon chilampoon closed this by deleting the head repository Sep 16, 2023
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.

2 participants