UCB's First PR After Requirements Discussion

[pharma-dev-hub]

In this update, we made a single change: we enabled the option to modify the gender ratio. By default, it is set to 0.52, which matches the value from the original branch.

We also addressed the other requirements that were previously mentioned.

Please review the changes and let us know your feedback.

Best regards,
Andras

[github-actions]

✅ All contributors have signed the CLA
_{Posted by the CLA Assistant Lite bot.}

[pharma-dev-hub]

Going to fix this CLA thing soon, thanks, Andras

[pharma-dev-hub]

recheck

[pharma-dev-hub]

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

[Alexandra01]

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

[Melkiades]

Thanks for the addition :) It is good to go for me.

There is a vignette that shows how to update the cached data. Only changing the generator will not change the cache.

How do you want to proceed with that? do you need the internal cache to be updated? @shajoezhu @BFalquet fyi

[shajoezhu]

hi @pharma-dev-hub , yes the cache will need updating as result of changing code, you are likely need to update the snapshot as well.

for this particular PR, it is changing the ratio of patient SEX,

once you update the cached data, we will need to assess downstream impact in scda.test and tlg-catalog public version (@pawelru FYI), I don't think we will have other impact on other nest packages,

Thank you!

[Alexandra01]

Hi @Melkiades, @shajoezhu,

I have run the script to update the cached data and no differences were found and so no data has been updated.

I think this is because the hard-coded proportion was 0.52 and the new argument has a default of 0.52, so while this argument has not been specified in the generation of cached data, the proportion is still the same.

[shajoezhu]

hi @Alexandra01 , can you please update the snapshots, it seems the date time is slightly off and it shows difference at the moment.

also, can you please fix the lintr issue. thank you

[Alexandra01]

Hi @shajoezhu yes, I will update the snapshots.
For the lintr issues, they are caused by files that haven't been changed in this PR (.pre-commit-config.yaml, _pkgdown.yml etc.) I just wanted to double check you still want the fix to be in this PR as it seems like an unrelated issue?

[kasaandras]

Hi Guys, Could we please get some feedback on Alexandra’s question? :)

[shajoezhu]

hi @Alexandra01 , sorry, missed the notification. yes, please can you fix the lintr issue, so we can make all checks green. it is very minor. Thanks!

[kasaandras]

recheck

[kasaandras]

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

[Alexandra01]

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

[kasaandras]

Hi all,

We are running into test failures because of timezone differences. For example, Alexandra is in the UK and I am in CET, so times like 01:00 versus 00:00 can cause issues when pushing changes.

Do you have a preferred way to handle this?

Should we force UTC in the tests, like:

format(Sys.time(), tz = "UTC")
Or set it globally in the test setup?
Sys.setenv(TZ = "UTC")

Thank you for the help.

[Melkiades]

Hi all,

We are running into test failures because of timezone differences. For example, Alexandra is in the UK and I am in CET, so times like 01:00 versus 00:00 can cause issues when pushing changes.

Do you have a preferred way to handle this?

Should we force UTC in the tests, like:

format(Sys.time(), tz = "UTC") Or set it globally in the test setup? Sys.setenv(TZ = "UTC")

Thank you for the help.

That is interesting! I think @shajoezhu must have a solution. He is based in a different timezone from central Europe. I would say to align to the current tests values on your systems. Maybe @cicdguy knows how this is set in the PR workflow?

[BFalquet]

I think you have to use something like

format_date <- function(date_format = "%d%b%Y") {
  function(x, ...) {
    toupper(
      format(
        # Extract the date at the location of the measure, not at the location of the system.
        lubridate::force_tz(x, tzone = "UTC"),
        date_format,
        tz = "UTC"
      )
    )
  }
}

or in the test, you could try withr::with_timezone

cheers

[Alexandra01]

Hi all,
Because the test file has over 200 lines, wrapping it all with withr::with_timezone seemed like a bit bulky, so I was thinking of updating the file to be:

current_timezone <- Sys.timezone()
Sys.setenv(TZ = "UTC")
.... {tests} ....
Sys.setenv(TZ = current_timezone)

Would that be an okay solution?

[BFalquet]

Hi all, Because the test file has over 200 lines, wrapping it all with withr::with_timezone seemed like a bit bulky, so I was thinking of updating the file to be:

current_timezone <- Sys.timezone()
Sys.setenv(TZ = "UTC")
.... {tests} ....
Sys.setenv(TZ = current_timezone)

Would that be an okay solution?

In that case, you could just place withr::local_timezone("UTC") at the beginning of the test and that should do the trick, e.g.

test_that("something", {
  print(Sys.time())
  withr::local_timezone("NZ")
  print(Sys.time())
  expect_true(TRUE)
})
#> [1] "2025-07-10 11:30:48 CEST"
#> [1] "2025-07-10 21:30:48 NZST"
#> Test passed 

[pawelru]

You can also have a package-level setting (as opposed to script-level setting) if you create setup.R file.
Docs: https://testthat.r-lib.org/articles/test-fixtures.html#package

[Melkiades]

do not worry about CLA, we can merge it anyway ;)

[Alexandra01]

@Melkiades I think there is a bigger issue going on here as I added the time zone fix and it works locally for both me and Andras, yet the tests are still failing in the check.
For example, the check brings up the following failed test because it expects the time to be 01:00:00 (from the snapshot) but the function produces 00:00:00.

          ADTM                  ADY AVISIT           AVISITN
        <dttm>              <int> <fct>              <int>
  -   1 2021-10-13 01:00:00   269 CYCLE 2 DAY 1          2
  +   1 2021-10-13 00:00:00   269 CYCLE 2 DAY 1          2

but the file row_values.md has the following values, specifically the time is 00:00:00

          ADTM                  ADY AVISIT           AVISITN
        <dttm>              <int> <fct>              <int>
      1 2021-10-13 00:00:00   269 CYCLE 2 DAY 1          2

So the test shouldn't expect the time to be 01:00:00 in the snapshot, and therefore the tests should pass.
Please correct me if I have this completely wrong!

[shajoezhu]

@Melkiades I think there is a bigger issue going on here as I added the time zone fix and it works locally for both me and Andras, yet the tests are still failing in the check. For example, the check brings up the following failed test because it expects the time to be 01:00:00 (from the snapshot) but the function produces 00:00:00.

          ADTM                  ADY AVISIT           AVISITN
        <dttm>              <int> <fct>              <int>
  -   1 2021-10-13 01:00:00   269 CYCLE 2 DAY 1          2
  +   1 2021-10-13 00:00:00   269 CYCLE 2 DAY 1          2

but the file row_values.md has the following values, specifically the time is 00:00:00

          ADTM                  ADY AVISIT           AVISITN
        <dttm>              <int> <fct>              <int>
      1 2021-10-13 00:00:00   269 CYCLE 2 DAY 1          2

So the test shouldn't expect the time to be 01:00:00 in the snapshot, and therefore the tests should pass. Please correct me if I have this completely wrong!

Thanks for reporting this.

hi @Alexandra01 , could you test something for me, if you change the timezone to "CET" timezone, would that resolve the snapshot issue?

[cicdguy]

do not worry about CLA, we can merge it anyway ;)

Yes exactly. Do not worry about the CLA. It is not mandatory for merging.

[Melkiades]

@Alexandra01 @kasaandras I think you have an old version here (I checked the fork). It would need to be rebased and updated with the upstream main/origin (this repo). I am adding a way that allows the update of data cache also from forks. Still needs some tuning. #329

As there are not so many modifications, I can make another PR with your mods later. Let me know if this PR contains everything you want to add now ;)

[shajoezhu]

hi @kasaandras @pharma-dev-hub @Alexandra01 can you try update your branch? I think Davide had fixed the issue for you. thanks

[kasaandras]

@shajoezhu @Melkiades @Alexandra01
We are confused about what exactly we need to do here. We have synced our fork to the latest version. Do you mean that Davide has fixed the issue and we can leave this pull request abandoned? Or what exactly do you mean here?

Please help me understand. It would be great to finalize this pull request eventually.
Andras

[shajoezhu]

hi @kasaandras , Davide @Melkiades is change was passing (I believe there were the changes that you would like to include into the package) , and with additional changes to allow checks to run. so I merged it in. but it seems at the moment, the main is failing, leave this with us. @Melkiades @edelarua , can you guys help fix the main please.

once the main is fixed. I suggest that we set a call together and discuss what else is needed, and how to collaborate. @pawelru @Polkas fyi

for now, let @Melkiades and @edelarua fix the main. thanks!

[shajoezhu]

hi @kasaandras @Alexandra01 , fyi the main is now fixed.

[shajoezhu]

I think we can close this PR now?

[shajoezhu]

approve and close the PR

[Melkiades]

There is nothing to merge in. I think we can consider the above PR as completed 👍