Skip to content

Unify picks feature branch: default parameters#1533

Merged
osenan merged 45 commits into
279-interactive_variables@mainfrom
default_values2@279-interactive_variables@main
May 18, 2026
Merged

Unify picks feature branch: default parameters#1533
osenan merged 45 commits into
279-interactive_variables@mainfrom
default_values2@279-interactive_variables@main

Conversation

@averissimo

@averissimo averissimo commented May 15, 2026

Copy link
Copy Markdown
Contributor

Pull Request

Changes description

  • Add teal.picks prefix
  • Restore some of the previous defaults
  • Change tm_t_tte to single method

averissimo and others added 30 commits May 14, 2026 18:01
@averissimo
fix: conf_values simplified as they are indirectly used
8569fcf
@averissimo
fix: remove arg_name
3ea7db6
@averissimo
fix: remove assertion and adds migration
c1de7d4
@averissimo
docs: update
d22c24a
@averissimo
revert some of the parameters
f46da8e
@averissimo
Apply suggestion from @averissimo
8979231 
Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
@averissimo
pr: feedback from @osenan
3740ad7
@averissimo @osenan
Apply suggestion from @osenan
4fc4063 
Co-authored-by: Oriol Senan  <35930244+osenan@users.noreply.github.com>
Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
@averissimo
pr: feedback from @osenan
e592e50
@averissimo
Merge branch '279-interactive_variables@main' into default_values@279…
7592e8f 
…-interactive_variables@main

Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
@averissimo
pr: feedback from @osenan
a968f6f
@averissimo
pr: feedback from @osenan
4b23cc8
@averissimo
pr: feedback from @osenan
0075ecd
@averissimo
Merge remote-tracking branch 'origin/default_values@279-interactive_v…
267084a 
…ariables@main' into default_values2@279-interactive_variables@main

* origin/default_values@279-interactive_variables@main:
  pr: feedback from @osenan
  pr: feedback from @osenan
  pr: feedback from @osenan
  Apply suggestion from @osenan
  Apply suggestion from @averissimo
  feat(picks): tm_g_barchart_simple (#1512)
@averissimo
Merge branch '279-interactive_variables@main' into default_values2@27…
9f01cfd 
…9-interactive_variables@main

* 279-interactive_variables@main:
  docs: update
  Unifying picks feature branch: `conf_level` standardization & `arg_name` removal (#1530)
@averissimo
fix: revert default parameters in module
f6f4068
@averissimo
fix: revert default parameters in module
f1b67a4
@averissimo
fix: revert default parameters in module
8eb76bd
@averissimo
fix: revert default parameters in module
9d2ce61
@averissimo
fix: revert default parameters in module
e2c245a
@averissimo
fix: revert default parameters in module
23c709a
@averissimo
fix: revert default parameters in module
df3ecda
@averissimo
fix: revert default parameters in module
690fc28
@averissimo
fix: revert default parameters in module
7735fa5
@averissimo
fix: revert default parameters in module
5cd0633
@averissimo
fix: revert default parameters in module
aa6276c
@averissimo
fix: revert default parameters in module
14ab2b8
@averissimo
fix: revert default parameters in module
0a7e41c
@averissimo
fix: revert default parameters in module
3f7d25d
@averissimo
fix: revert default parameters in module
8aff817
m7pr
m7pr reviewed May 15, 2026
View reviewed changes
Comment thread R/tm_g_ipp.R
m7pr
m7pr reviewed May 15, 2026
View reviewed changes
Comment thread R/tm_g_ci_picks.R
),
x_var,
y_var,
color,

@m7pr m7pr May 15, 2026

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.

At some point I though that for S3 dispatches for teal.picks we will just provide teal.picks default.
No idea if this is a good approach or if it's even possible to guess proper defaults, so maybe we can revert back to empty values

@averissimo averissimo May 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I guess picks should the default then, I'll make the change there

@m7pr m7pr May 15, 2026

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.

thanks!

@averissimo averissimo May 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, please review @m7pr

@averissimo averissimo May 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note: can you update the documentation field on event_desc_var I'm not in the context on why wasreferencing an internal implementation

m7pr
m7pr reviewed May 15, 2026
View reviewed changes
Comment thread R/tm_t_abnormality.R
m7pr
m7pr reviewed May 15, 2026
View reviewed changes
Comment thread R/tm_t_coxreg.R Outdated
m7pr
m7pr reviewed May 15, 2026
View reviewed changes
Comment thread R/tm_t_events_patyear.R Outdated
m7pr
m7pr reviewed May 15, 2026
View reviewed changes
Comment thread R/tm_t_tte.R
m7pr
m7pr reviewed May 15, 2026
View reviewed changes
Comment thread NAMESPACE
m7pr
m7pr reviewed May 15, 2026
View reviewed changes

@m7pr m7pr 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.

@averissimo overall looks really clean! just left a few verification questions, but they are really minor : )

@m7pr m7pr mentioned this pull request May 15, 2026
Merged
@m7pr

m7pr commented May 15, 2026

Copy link
Copy Markdown
Contributor

@averissimo one this is merged, I think we need to make a second round on this PR #1533 as some of the defaults changed :D

@m7pr

m7pr commented May 15, 2026

Copy link
Copy Markdown
Contributor

Hey @averissimo this is good, but

  • 14 tm_g_ci tests are failing : P
  • styler is failing
  • lintr is failing

styler and lintr were already fixed on the feature branch, so we need to fix them here as well

@averissimo

averissimo commented May 15, 2026

Copy link
Copy Markdown
Contributor Author

@m7pr all fixed now!

@averissimo
averissimo requested a review from m7pr May 15, 2026 15:37
@m7pr

m7pr commented May 15, 2026

Copy link
Copy Markdown
Contributor

@averissimo I can still see this in R CMD CHECK - probably you changed parameter default, but not the assertion

Initializing tm_t_shift_by_arm_by_worst
Error in tm_t_shift_by_arm_by_worst(label = "Shift by Arm Table", dataname = "ADEG",  : 
  Assertion on 'teal.picks::is_pick_multiple(treatment_flag)' failed: Must be FALSE.
Calls: init ... tm_t_shift_by_arm_by_worst -> <Anonymous> -> makeAssertion -> mstop
Execution halted

https://github.com/insightsengineering/teal.modules.clinical/actions/runs/25926843992/job/76210274063?pr=1533

@m7pr

m7pr commented May 16, 2026

Copy link
Copy Markdown
Contributor

Now I see some warning in R CMD CHECK

* checking Rd cross-references ... WARNING
Missing link(s) in Rd file 'tm_t_tte.Rd':
  ‘tm_t_tte.picks’ ‘tm_t_tte.variables’ ‘tm_t_tte.default’
  ‘tm_t_tte.list’

See section 'Cross-references' in the 'Writing R Extensions' manual.

Found the following Rd file(s) with Rd \link{} targets missing package
anchors:
  tm_t_tte.Rd: tm_t_tte.picks, tm_t_tte.variables, tm_t_tte.default,
    tm_t_tte.list

Maybe @osenan will know how to continue while we are gone.

@osenan

osenan commented May 18, 2026

Copy link
Copy Markdown
Contributor

The error was that the documentation referenced methods for tm_t_tte module that no longer existed and could not link them when creating the documentation pages. Removing the unnecessary references fixed the warning.

@osenan
osenan merged commit 755ea4f into 279-interactive_variables@main May 18, 2026
39 of 40 checks passed
@osenan
osenan deleted the default_values2@279-interactive_variables@main branch May 18, 2026 14:45
@github-actions github-actions Bot locked and limited conversation to collaborators May 18, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@osenan osenan Awaiting requested review from osenan
@m7pr m7pr Awaiting requested review from m7pr

Assignees

No one assigned

Labels

core

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@averissimo @m7pr @osenan