tidybulk icon indicating copy to clipboard operation
tidybulk copied to clipboard

Improve documentation and package imports

Open william-hutchison opened this issue 2 years ago • 26 comments

This pull request picks up on @chilampoon's pull request #297 . In summary:

  • Polish roxygen2 function documentation (e.g inheriting from other packages when possible)
  • Improve coding style (e.g "=" converted to "<-")
  • Switch Github actions workflow from check-bioc to rworkflows
  • Document function imports in roxygen2, NAMESPACE and DESCRIPTION instead of installing and loading packages within function calls
  • Update tidyomics ecosystem description in the README and vignette
  • Replace the example data used in the README

william-hutchison avatar Jan 09 '24 06:01 william-hutchison

I can help address errors in the checks over the weekend if they haven't been taken care of..

chilampoon avatar Jan 12 '24 04:01 chilampoon

I can help address errors in the checks over the weekend if they haven't been taken care of..

Thanks! Please do. And please coordinate with @william-hutchison. The resubmission is very imminent.

stemangiola avatar Jan 12 '24 04:01 stemangiola

Great, thanks for your help @chilampoon! Feel free to continue from here: https://github.com/william-hutchison/tidybulk/tree/add-package-installation-messages

The import system is a bit unconventional in that we are prompting the user to install and load large database packages through the function check_package_availability instead of installing them all automatically. This is working well everywhere except in the unit tests, where the packages must be explicitly installed and loaded (you can see an example of this at the top of the test-bulk_methods_SummarizedExperiment.R file.

So, part of what may be required is to manually install and load missing packages in this way for the unit tests to run.

I also saw multiple unit tests begin to fail after the merge at commit 989009a which will probably be the main challenge to solve.

william-hutchison avatar Jan 12 '24 06:01 william-hutchison

@william-hutchison how are we going with this PR?

We have a lot of PR open and we should try to complete them for this Bioc release.

stemangiola avatar Feb 06 '24 23:02 stemangiola

Hi @stemangiola, yes I agree. @chilampoon, are you still planning on having a go at fixing the remaining few errors? If not I will try to find some time to finish this off over the next week or so.

william-hutchison avatar Feb 07 '24 04:02 william-hutchison

Hi @stemangiola, yes I agree. @chilampoon, are you still planning on having a go at fixing the remaining few errors? If not I will try to find some time to finish this off over the next week or so.

@chilampoon, if you could push this true, it would be amazing. Please, let us know because we are getting the repository ready for the next Bioconductor release..

stemangiola avatar Feb 07 '24 04:02 stemangiola

sure thing. When I was looking at the errors, I found some of them are not easy to fix although seem trivial. I can fix those relatively easier-to-fix errors by the end of this week and @william-hutchison to fix the rest of them? your understanding of the underlying architecture and unit tests should be much better than mine..

chilampoon avatar Feb 07 '24 05:02 chilampoon

Thanks @chilampoon! Yep sounds like a plan, see how you go and I will attempt to fix whatever remains.

william-hutchison avatar Feb 07 '24 22:02 william-hutchison

Thanks @chilampoon! Yep sounds like a plan, see how you go and I will attempt to fix whatever remains.

hi @william-hutchison can you add me as a contributor to your tidybulk repo? thx

chilampoon avatar Feb 11 '24 06:02 chilampoon

Thanks @chilampoon! Yep sounds like a plan, see how you go and I will attempt to fix whatever remains.

hi @william-hutchison can you add me as a contributor to your tidybulk repo? thx

Hello @chilampoon we interact with forks for the tidyomics. Is much better to track who has contributed to what and see the repo history.

stemangiola avatar Feb 11 '24 08:02 stemangiola

Thanks @chilampoon! Yep sounds like a plan, see how you go and I will attempt to fix whatever remains.

hi @william-hutchison can you add me as a contributor to your tidybulk repo? thx

Hello @chilampoon we interact with forks for the tidyomics. Is much better to track who has contributed to what and see the repo history.

I meant his forked tidybulk repo so I can directly add commits to this branch

chilampoon avatar Feb 11 '24 14:02 chilampoon

@chilampoon, sounds good. I have added you as a collaborator to my fork.

william-hutchison avatar Feb 11 '24 22:02 william-hutchison

@william-hutchison please update all forks from my master (I have solved the ERROR in the tests).

stemangiola avatar Feb 12 '24 01:02 stemangiola

@william-hutchison please update all forks from my master (I have solved the ERROR in the tests).

Pleaser @chilampoon pull again from my master. I have fixed a bug.

stemangiola avatar Feb 12 '24 02:02 stemangiola

@william-hutchison please update all forks from my master (I have solved the ERROR in the tests).

Pleaser @chilampoon pull again from my master. I have fixed a bug.

it's in this commit Merge remote-tracking branch 'stemangiola/master' into ...

chilampoon avatar Feb 12 '24 03:02 chilampoon

@william-hutchison please update all forks from my master (I have solved the ERROR in the tests).

Pleaser @chilampoon pull again from my master. I have fixed a bug.

it's in this commit Merge remote-tracking branch 'stemangiola/master' into ...

Needs to be done again :)

stemangiola avatar Feb 12 '24 03:02 stemangiola

Hello @chilampoon, I hope you're well. Any news with this project? We have put a lot of energies, it would be worth to take it to completion.

stemangiola avatar Mar 06 '24 11:03 stemangiola

Hello @chilampoon, I hope you're well. Any news with this project? We have put a lot of energies, it would be worth to take it to completion.

yes lets get it done. Re the package dependencies - so many functionalities are included in this package, I wonder if it's possible to remove some of the non-core functions, especially those for running very downstream tasks? instead we could just keep some examples in the vignette, then the package would be more lightweight

chilampoon avatar Mar 10 '24 03:03 chilampoon

Hello @chilampoon, I hope you're well. Any news with this project? We have put a lot of energies, it would be worth to take it to completion.

yes lets get it done. Re the package dependencies - so many functionalities are included in this package, I wonder if it's possible to remove some of the non-core functions, especially those for running very downstream tasks? instead we could just keep some examples in the vignette, then the package would be more lightweight

Sure we can have this discussion. However better we do one thing at the time.

@chilampoon I see the problem here i that the Github do not pass. I think we are using a not efficient way to approach this problem. Are you doing devtools::check() locally before pushing to the GiHub?

We might be close, but I feel we are stuck on this loop. Do you feel you would like a brief meeting on the best testing checking strategy?

or @william-hutchison do you feel it would be easier for you to bring this PR to completion?

stemangiola avatar Mar 10 '24 23:03 stemangiola

Yes I run devtools::check() and R CMD CHECK before committing, there are still errors/warnings I haven't addressed. Sure I am available to meet on Thursday & Friday, my time is EDT @stemangiola

chilampoon avatar Mar 11 '24 18:03 chilampoon

Hello @chilampoon, sorry for the delay in grant writing time. Any chance we could solve this offline? @william-hutchison, maybe you could advise on why checks are still failing.

stemangiola avatar May 15 '24 13:05 stemangiola

Hello @chilampoon, sorry for the delay in grant writing time. Any chance we could solve this offline? @william-hutchison, maybe you could advise on why checks are still failing.

Hi yeah I have been preparing for my candidacy exam which will happen in two weeks, maybe @william-hutchison could help point out the issues by then? And I can meet on zoom in the second week of June

chilampoon avatar May 16 '24 22:05 chilampoon

@arashbioinfo decided to try to solve this PR. @william-hutchison can give you an hand if you get stuck.

@arashbioinfo, the first thing is to solve the "conflicts", there are plenty of tutorial on how to deal with them. Conflict emerge when the master branch diverges a lot from the pool request.

Maybe @william-hutchison , could you solve the conflicts since you have more background on the package, then @arashbioinfo will try to solve the github checks

stemangiola avatar Aug 26 '24 00:08 stemangiola

@arashbioinfo decided to try to solve this PR. @william-hutchison can give you an hand if you get stuck.

@arashbioinfo, the first thing is to solve the "conflicts", there are plenty of tutorial on how to deal with them. Conflict emerge when the master branch diverges a lot from the pool request.

Maybe @william-hutchison , could you solve the conflicts since you have more background on the package, then @arashbioinfo will try to solve the github checks

Thank you, @stemangiola, for the guidance and support. I appreciate the opportunity to contribute to this PR.

@william-hutchison, thank you in advance for your help. If possible, could we arrange a brief meeting to discuss the best approach to resolving this PR and aligning our efforts?

arashabadi avatar Aug 26 '24 05:08 arashabadi

Hi @arashbioinfo, sure. Would you like to email me at [email protected] to arrange a time? I'm looking forward to working with you on this!

william-hutchison avatar Aug 26 '24 05:08 william-hutchison