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.

Sign up for GitHub

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

Jump to bottom

0015 derive seq #53

Merged
merged 109 commits into from
May 30, 2024
Merged

0015 derive seq #53

merged 109 commits into from
May 30, 2024

Conversation

ramiromagno
Copy link
Collaborator

Thank you for your Pull Request! We have developed this task checklist from the
Development Process
Guide
to help with the final steps of the process. Completing the below tasks helps to
ensure our reviewers can maximize their time on your code as well as making sure
the oak codebase remains robust and consistent.

Please check off each taskbox as an acknowledgment that you completed the task
or check off that it is not relevant to your Pull Request. This checklist is
part of the Github Action workflows and the Pull Request will not be merged into
the devel branch until you have checked off each task.

  • Place Closes #<insert_issue_number> into the beginning of your Pull z
    Request Title (Use Edit button in top-right if you need to update)
  • Code is formatted according to the
    tidyverse style guide. Run
    styler::style_file() to style R and Rmd files
  • Updated relevant unit tests or have written new unit tests, which should
    consider realistic data scenarios and edge cases, e.g. empty datasets, errors,
    boundary cases etc. - See
    Unit Test Guide
  • If you removed/replaced any function and/or function parameters, did you
    fully follow the
    deprecation guidance?
  • Update to all relevant roxygen headers and examples, including keywords
    and families. Refer to the
    categorization of functions to tag appropriate keyword/family.
  • Run devtools::document() so all .Rd files in the man folder and the
    NAMESPACE file in the project root are updated appropriately
  • Address any updates needed for vignettes and/or templates
  • Update NEWS.md if the changes pertain to a user-facing function (i.e. it
    has an @export tag) or documentation aimed at users (rather than developers)
  • Build oak site pkgdown::build_site() and check that all affected
    examples are displayed correctly and that all new functions occur on the "Reference" page.
  • Address or fix all lintr warnings and errors - lintr::lint_package()
  • Run R CMD check locally and address all errors and warnings - devtools::check()
  • Link the issue in the Development Section on the right hand side.
  • Address all merge conflicts and resolve appropriately
  • Pat yourself on the back for a job well done! Much love to your accomplishment!

ramiromagno and others added 30 commits February 8, 2024 02:22
@ramiromagno
First mockup of hardcode_no_ct()
1c4501f
@ramiromagno
Update hardcode_no_ct()
faef0b1 
Update `hardcode_no_ct()` by allowing the rewriting of the `target_sdtm_variable` variable to preserve `NA`
@ramiromagno
Align hardcode_no_ct() code style with Ramm's expectations
fd63b37
@ramiromagno
Add hardcode_*() and assign_*() functions
80d3943
@rammprasad
hardcode_no_ct algorithm code changes (#45)
ec5a9e4 
* hardcode_no_ct algorithm code changes

* harcode_ct working as expected

* assign_ct and assign_no_ct works great.

* address review comments
@ramiromagno
Add oak_id_vars()
0333d95
@ramiromagno
Fix typo in recode()
7fd7716
@ramiromagno
Simplify oak_id_vars() docs
802aacc
@ramiromagno
Update assign_* and hardcode_* implementations
7fd07b4
@ramiromagno
Introduce memoisation of ct_mappings()
9cc26d1
@ramiromagno
Update of README introductory paragraph
329feaa
@ramiromagno
Merge from main
29be830 
Merge branch 'main' into 0040_hardcode_no_ct

# Conflicts:
#	DESCRIPTION
#	NAMESPACE
#	inst/WORDLIST
@ramiromagno
Update hardcode_* functions' interface
7720c05
@ramiromagno
Add contains_oak_id_vars() function
e87ca66
@ramiromagno
Update contains_oak_id_vars() doc examples
a5e61f0
@ramiromagno
Update sdtm_harcode() and dependant functions
a60ccd6
@ramiromagno
Update assign_* and hardcore_* related functions
cd89804
@ramiromagno @pharmaverse-bot
Automatic renv profile update.
ae2da80
@ramiromagno @pharmaverse-bot
Automatic renv profile update.
30857e3
@ramiromagno
Make ct and cl parameters mandatory for assign_ct()
73ebe2d
@ramiromagno
Add functions ct importing
0eb4677 
- Adds three new user facing ct-related functions: `read_ct_example()`, `ct_example()` and `read_ct()`
- Provides a ct example file in inst/ct/
@ramiromagno
Bring hardcode*() and assign*() related assertions closer to user…
dfd7710 
… calling functions
@ramiromagno
Add lagging behind Rd for ct_example()
6652aae
@ramiromagno
Add assert_ct()
59bcc71
@ramiromagno
Add ct assertions
7f9f388
@ramiromagno
Merge branch '0040_hardcode_no_ct' of github.com:pharmaverse/sdtm.oak…
4c81ae1 
… into 0040_hardcode_no_ct
@ramiromagno
Remove R/.gitkeep
4ed5c41 
As it is no longer needed.
@ramiromagno
Add unit tests for ct_vars()
ca26d22
@ramiromagno
Update dependencies
0456d55
@ramiromagno
Export ct_vars()
0e1eab4 
Export `ct_vars()` such that we can cross-reference it from other functions' documentation.
@ramiromagno
First mockup of derive_seq()
b7d104d
@ramiromagno
Simplification of derive_seq() interface
58688df 
`derive_seq()` no longer has a default value for parameter `id_vars` as per the meeting of 2024-05-15, so function's logic has been simplified accordingly.
@ramiromagno ramiromagno self-assigned this May 15, 2024
@ramiromagno
Copy link
Collaborator Author

ramiromagno commented May 15, 2024 •
edited
Loading

@edgar-manukyan: As discussed today, I will wait for those examples for inclusion in the Examples section of the docs and to create unit tests. Many thanks!

@ramiromagno ramiromagno linked an issue May 15, 2024 that may be closed by this pull request
Develop a function to generate --SEQ. #15
Closed
@github-actions GitHub Actions
Copy link

github-actions bot commented May 15, 2024 •
edited
Loading

Code Coverage

Package Line Rate Health
sdtm.oak 68% ➖
Summary 68% (498 / 734) ➖

ramiromagno and others added 5 commits May 15, 2024 17:43
@ramiromagno
Bump dev version and update NEWS
a56d378
@ramiromagno @pharmaverse-bot
Automatic renv profile update.
b99cb9d
@ramiromagno
Update derive_seq()
8662fa7 
- Two examples of domain data sets were added: vs and apsc
- An internal data set `domain_record_vars` provides the mapping between domain names and key variables (thank you @edgar-manukyan)
- `rec_vars()` was added to access those record-defining variables
- Likewise, `sbj_vars()` for subject-level defining rows
@ramiromagno
Merge branch '0015_derive_seq' of github.com:pharmaverse/sdtm.oak int…
c4ea73d 
…o 0015_derive_seq
@ramiromagno @pharmaverse-bot
Automatic renv profile update.
b42e7be
rammprasad
rammprasad requested changes May 21, 2024
View reviewed changes
Copy link
Collaborator

@rammprasad rammprasad left a comment

Choose a reason for hiding this comment

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

Overall function works as expected. We just need to look into the data-raw folder and keep just the ones we need.

R/derive_seq.R Outdated Show resolved Hide resolved
data-raw/pharmaversesdtm.R Outdated Show resolved Hide resolved
data-raw/roak_ex.R Outdated Show resolved Hide resolved
@ramiromagno ramiromagno requested a review from rammprasad May 22, 2024 10:13
rammprasad
rammprasad approved these changes May 30, 2024
View reviewed changes
Copy link
Collaborator

@rammprasad rammprasad left a comment

Choose a reason for hiding this comment

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

Thank you for addressing the comments.

@rammprasad
Copy link
Collaborator

@edgar-manukyan - Ready for your review.

edgar-manukyan
edgar-manukyan approved these changes May 30, 2024
View reviewed changes
Copy link
Collaborator

@edgar-manukyan edgar-manukyan left a comment

Choose a reason for hiding this comment

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

Thanks so much Ramiro, looks fine to me. Just left two comments in case you want to address them. Are we going to add tests at some point?

R/derive_seq.R Show resolved Hide resolved
R/derive_seq.R Outdated Show resolved Hide resolved
@ramiromagno
Copy link
Collaborator Author

Thanks so much Ramiro, looks fine to me. Just left two comments in case you want to address them. Are we going to add tests at some point?

I am happy to write the unit tests, but could you provide some input and output examples such that the tests are based on realistic examples, and also cover corner cases. (you guys have more experience here).

@edgar-manukyan
Copy link
Collaborator

Thanks so much Ramiro, looks fine to me. Just left two comments in case you want to address them. Are we going to add tests at some point?

I am happy to write the unit tests, but could you provide some input and output examples such that the tests are based on realistic examples, and also cover corner cases. (you guys have more experience here).

Ram mentioned that we can have a separate issue to increase the coverage, no worries Ramiro. Thanks so much for your brilliant work 💠 💠 💠

@ramiromagno
Addressing Edgar's review
fec01fc 
- `derive_seq()` checks the name of `tgt_var`
- Added comment to clarify the code
@ramiromagno
Add sbj_vars() to pkgdown reference list
1aaa289
@ramiromagno ramiromagno merged commit e1aa479 into main May 30, 2024
15 checks passed
@ramiromagno ramiromagno deleted the 0015_derive_seq branch May 30, 2024 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edgar-manukyan edgar-manukyan edgar-manukyan approved these changes

@rammprasad rammprasad rammprasad approved these changes

Assignees

@ramiromagno ramiromagno

Labels
None yet
Projects
sdtm.oak R package
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Develop a function to generate --SEQ.
3 participants
@ramiromagno @rammprasad @edgar-manukyan