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 tidyomics conflicts and docs clean up #297

Closed
wants to merge 9 commits into from
Closed

resolve tidyomics conflicts and docs clean up #297

wants to merge 9 commits into from

Conversation

chilampoon
Copy link

  • clean up and refactor docs majorly in dplyr_methods.R and tidyr_methods.R and some in funcitons(_se).R and methods(_se).R

  • I got notes when install the package so then tried to fix them

    Note: wrong number of arguments to 'expm1' at methods_SE.R:898 
    Note: wrong number of arguments to 'floor' at functions_SE.R:363 
    
  • resolve warnings in some tests from testthat due to different adj pvalues . I am not sure if it's because of using a newer version of TMB package

  • resolve warnings:

    WARNING
    '::' or ':::' import not declared from: ‘pkgconfig’
    Unexported objects imported by ':::' calls:
    ‘glmmSeq:::glmmTMBcore’ ‘glmmSeq:::hyp_matrix’ ‘glmmSeq:::lmer_wald’
    ‘glmmSeq:::organiseStats’
    

    TBD: a lot of notes

(right now try to do CI tests to see if any issues pop up)

@stemangiola
Copy link
Owner

for glmmSeq. i solved all those. i will push the changes in 3 hours

@chilampoon
Copy link
Author

@stemangiola sounds good.
^in that commit I hope to avoid install errors in workflow tests, lets see..

@stemangiola
Copy link
Owner

@stemangiola sounds good. ^in that commit I hope to avoid install errors in workflow tests, lets see..

This glmmSeq PR should fix things up

#296

@chilampoon
Copy link
Author

@stemangiola There's still this warning in workflow CMD check

❯ 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.

There is one filter.tidybulk in R/dplyr-methods.R and one in dev/dplyr-master-methods.R.
I added a .Rinsignore to ignore files in dev/ but it doesn't work. Now I deleted the master methods script to see if this is the source of waning...

Here are the possibilities listed by chatgpt:

  • Explicit Export: Ensure that filter.tidybulk is correctly exported in your NAMESPACE file.
  • Rd Documentation: Make sure that the documentation (@export) for filter.tidybulk exists and is correctly formatted.
  • Correct S3 Registration: Ensure that your NAMESPACE file includes the line S3method(filter,tidybulk).
  • Rebuild and Recheck: Sometimes it helps to clean the workspace, rebuild the documentation, and then check again.
  • Check Locally: Before pushing to GitHub, you might want to run devtools::check() locally to identify and fix any errors or warnings.
  • Workflow File: Ensure your GitHub Actions workflow file (or whatever CI system you're using) is set up to install all dependencies and any required system libraries.
  • Other Files in dev/: Make sure that there aren't other files in the dev/ directory that the package relies upon, especially since you've instructed R to ignore everything in that directory.

I don't get this warning in local check so need to push commits here...

@stemangiola
Copy link
Owner

@chilampoon I believe @noriakis solved the same problem in tidySummarizedExperiment. @noriakis could you please give advise?

@noriakis
Copy link

Thank you for the mention! The warning in the method lookup could be solved by moving dplyr to Depends from Imports (https://stat.ethz.ch/pipermail/r-devel/2014-June/069247.html). Also, it may come from R versioning, as I can pass R CMD CHECK without errors and warnings in R 4.3.1 (and Bioc 3.17) based on the commit. So updating check-bioc.yml to check in R >= 4.3 would also solve the warnings without moving dplyr to Depends.

@stemangiola
Copy link
Owner

Thank you for the mention! The warning in the method lookup could be solved by moving dplyr to Depends from Imports (https://stat.ethz.ch/pipermail/r-devel/2014-June/069247.html). Also, it may come from R versioning, as I can pass R CMD CHECK without errors and warnings in R 4.3.1 (and Bioc 3.17) based on the commit. So updating check-bioc.yml to check in R >= 4.3 would also solve the warnings without moving dplyr to Depends.

Thanks! lets use the same github action as tidySingleCellExperiment and tidySummarizedExperiment then

@chilampoon
Copy link
Author

@stemangiola there are always new errors/warnings show up after fixing something... I think I better move on clearing up other docs rather than focusing on the RCMDcheck results...

@stemangiola
Copy link
Owner

@stemangiola there are always new errors/warnings show up after fixing something... I think I better move on clearing up other docs rather than focusing on the RCMDcheck results...

Yes there are few errors and warnings. I think they are easy to fix, but if you have something in mind feel free to suggest. I think it is better that you run github action on your master branch (so you don't need authorisation) and when they are warning free you can push to the main master.

Yes tidybulk needs some work as it is the biggest and oldest repository of tidy transcriptomics.

@chilampoon
Copy link
Author

@stemangiola there are always new errors/warnings show up after fixing something... I think I better move on clearing up other docs rather than focusing on the RCMDcheck results...

Yes there are few errors and warnings. I think they are easy to fix, but if you have something in mind feel free to suggest. I think it is better that you run github action on your master branch (so you don't need authorisation) and when they are warning free you can push to the main master.

Yes tidybulk needs some work as it is the biggest and oldest repository of tidy transcriptomics.

Sounds good, close this PR for now. Thanks @noriakis and @stemangiola !

@chilampoon chilampoon closed this Oct 3, 2023
@william-hutchison
Copy link

Hi @chilampoon, just letting you know I am making some changes from commit #1a0c713 so that we don't duplicate our efforts.

I am hoping to get this passing BiocCheck and R CMD CHECK (currently fixing existing issue in tidybulk as far as I can tell, not related to your improvements) in the next few days so that we can merge before too many changes are made to the main branch.

You can see my changes here https://github.com/william-hutchison/tidybulk/tree/chilampoon-improve-documentation . Let me know if you have any suggestions and thanks for your work!

@chilampoon
Copy link
Author

Hi @william-hutchison looks good to me! Please keep me posted when you finish the updates

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.

4 participants