reimandlab / ActivePathways

Integrative pathway enrichment analysis of multivariate omics data
101 stars 23 forks source link

ERROR (test_merge_p_values.r:43:5): Merged p-values are correct #14

Closed mattdowle closed 2 years ago

mattdowle commented 3 years ago

Hi, Thanks for using and importing data.table. In reverse dependency testing of the next data.table release, I'm running R CMD check for ActivePathways and seeing the following error. It's the same one currently occurring on CRAN just on r-oldrel-macos, oddly. https://www.r-project.org/nosvn/R.check/r-oldrel-macos-x86_64/ActivePathways-00check.html And here's my output: 00check.log Would installing the Suggested package EmpiricalBrownsMethod make this test pass, perhaps? Thanks, Matt

mattdowle commented 3 years ago

I installed EmpiricalBrownsMethod from Bioconductor and that indeed solved it. Passes R CMD check fully OK now. I see that the r-oldrel-macos log on CRAN also shows that it doesn't have EmpiricalBrownsMethod installed.

I'll leave this issue open just in case you want to tweak the test to cope better when that package isn't installed.

reimand0 commented 3 years ago

Hi Matt,

thanks for getting in touch. We implemented the few bits from EmpiricalBrownsMethod within our package because that package had disappeared from CRAN, if I recall correctly. Is it correct that the issue currently only affects older versions of ActivePathways?

best, Jüri

On Thu, Dec 3, 2020 at 3:48 PM Matt Dowle notifications@github.com wrote:

EXTERNAL EMAIL:

I installed EmpiricalBrownsMethod from Bioconductor and that indeed solved it. Passes R CMD check fully OK now. I see that the r-oldrel-macos log on CRAN also shows that it doesn't have EmpiricalBrownsMethod installed.

I'll leave this issue open just in case you want to tweak the test to cope better when that package isn't installed.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/reimandlab/ActivePathways/issues/14#issuecomment-738298137, or unsubscribe https://github.com/notifications/unsubscribe-auth/AETAF7M724OUDBOCDBXQP7DSS72SRANCNFSM4UMQFFZA .

mattdowle commented 3 years ago

Is it correct that the issue currently only affects older versions of ActivePathways?

Affects v1.0.2 as on CRAN, published 2020-07-09.

reimand0 commented 3 years ago

OK, found it. The package only affects testing since it is included to check the output of our custom code. Not sure what the best practices are to resolve this issue. Thoughts would be appreciated!

thanks, Jüri

On Fri, Dec 4, 2020 at 3:58 AM Matt Dowle notifications@github.com wrote:

EXTERNAL EMAIL:

Is it correct that the issue currently only affects older versions of ActivePathways?

Affects v1.0.2 as on CRAN, published 2020-07-09.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/reimandlab/ActivePathways/issues/14#issuecomment-738658895, or unsubscribe https://github.com/notifications/unsubscribe-auth/AETAF7MD7TOEXQ4YRE53BUTSTCQFBANCNFSM4UMQFFZA .

mattdowle commented 3 years ago

Great. What we do is attempt a call to require for each of our Suggested packages. A boolean variable test_<pkg> is then created which we use to switch off tests when the suggested package is not available. That's done here: https://github.com/Rdatatable/data.table/blob/master/inst/tests/tests.Rraw#L94 That suppression around require seems inelegant, looking at it now, and if you find an easier way please let me know; e.g. maybe an is.installed function exists somewhere. I imagine you would do a much simplified version of the way we do it. Alternatively, you could just take the position that all Suggests must be installed for your R CMD check to pass, and not do anything. It was just that I'm more used to seeing 'function not found' or something like that, which gives me the clue that I'm missing a suggested package. But in this case the strange error message sent me down a rabbit hole for a short time, the message about merge started me thinking about merge.data.table, when in fact all I had to do was deal with the missing suggest.

reimand0 commented 2 years ago

this has been resolved in the latest version - thanks for reporting!

mattdowle commented 2 years ago

Great. Thanks, @reimand0!