Skip to content

Added the exclude_rows argument to g_forest(). - #1504

Merged
shajoezhu merged 3 commits into
pharmaverse:mainfrom
wwojciech:1498_g_forest_exclude_rows
Aug 8, 2026
Merged

Added the exclude_rows argument to g_forest().#1504
shajoezhu merged 3 commits into
pharmaverse:mainfrom
wwojciech:1498_g_forest_exclude_rows

Conversation

@wwojciech

@wwojciech wwojciech commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Fixes #1498 (Replaces #1503 due to CI/CD issues).

@wwojciech

Copy link
Copy Markdown
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@wwojciech
wwojciech marked this pull request as draft August 8, 2026 08:06
@wwojciech
wwojciech marked this pull request as ready for review August 8, 2026 08:06
@wwojciech

wwojciech commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

Hi @shajoezhu, @Melkiades,

I created this new PR to replace #1503.

CI/CD is still failing, and I think there are two main reasons for this:

  1. The CLA setup has changed with the move to the new pharmaverse model.
  2. There are still some old URLs pointing to insightsengineering instead of the new pharmaverse organization.

Would it be possible for you to update the required files/configuration?

I created #1505 for that.

Thanks!

@danielinteractive , @gmbecker - FYI.

@shajoezhu

Copy link
Copy Markdown
Contributor

it is much better than before, we can fix the url in a seperate PR, that's not a blocker for us to merging this, if only cla check failing, we can fix that later. let's see the others

@wwojciech

wwojciech commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

it is much better than before, we can fix the url in a seperate PR, that's not a blocker for us to merging this, if only cla check failing, we can fix that later. let's see the others

Thank you @shajoezhu. If the remaining issues are not blockers, could you please approve this PR so that I can merge it?
PR #1503 was already approved by @danielinteractive and @Melkiades.

PS. I noticed two NOTES in the R CMD check:

  1. Example timing (g_forest())
    The example is just slightly above the 5-second threshold (user = 5.13s, elapsed = 5.143s). Since the added example is a simple one-line call that executes quickly, I suspect the examples were already close to the limit and this pushed it slightly over.
  2. checking CRAN incoming feasibility ... NOTE

@shajoezhu

Copy link
Copy Markdown
Contributor

cran check will not pass

PS. I noticed two NOTES in the R CMD check:

Example timing (g_forest())
The example is just slightly above the 5-second threshold (user = 5.13s, elapsed = 5.143s). Since the added example is a simple one-line call that executes quickly, I suspect the examples were already close to the limit and this pushed it slightly over.
checking CRAN incoming feasibility ... NOTE
This appears to be a separate CRAN incoming-check NOTE.

can you fix them

@wwojciech

wwojciech commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

cran check will not pass

PS. I noticed two NOTES in the R CMD check:

Example timing (g_forest()) The example is just slightly above the 5-second threshold (user = 5.13s, elapsed = 5.143s). Since the added example is a simple one-line call that executes quickly, I suspect the examples were already close to the limit and this pushed it slightly over. checking CRAN incoming feasibility ... NOTE This appears to be a separate CRAN incoming-check NOTE.

can you fix them

I don’t think I can fix these without revisiting the implementation of the existing functions/examples.

The g_forest() timing issue appears to be caused by an example that was already close to CRAN’s 5-second threshold, and the additional (1-line trivial) example pushed it slightly over. Fixing that properly would likely require changing the implementation or restructuring the existing examples, neither of which I originally wrote.

I also profiled the g_forest() example to identify the source of the timing issue. The main time-consuming operation is cowplot::plot_grid().

The incoming feasibility NOTE also appears to be separate from the added example and not something I can directly address.

@shajoezhu

Copy link
Copy Markdown
Contributor

skip the example if run on cran

@wwojciech

Copy link
Copy Markdown
Contributor Author

skip the example if run on cran

Hi @shajoezhu - done. R CMD checks is passsing now.

@shajoezhu shajoezhu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm!

@shajoezhu
shajoezhu merged commit a357a60 into pharmaverse:main Aug 8, 2026
27 of 29 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 8, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature Request]: Allow excluding rows from the g_forest().

2 participants