Refactor simd locality by LucyEmma22 · Pull Request #1334 · Public-Health-Scotland/source-linkage-files

LucyEmma22 · 2026-06-02T08:36:43Z

NOTES:

get_locality_data(), get_simd_data() and get_datazone_pop_data() are new functions based on get_spd_data() (see BYOC_ID: 45966 Refactor sc demographics #1313)

process_lookup_postcode() uses get_spd_data() which is refactored in another branch (refactor-sc-demog).

fill_geographies() uses get_slf_gpprac_path() which is refactored in another branch (refactor_gpprac_lookup).

TO-DO:

Resolve comments
Merge with branches containing refactored get_spd_data() and get_slf_gpprac_path()
Check SDL table names
Run with data

…functions to get_lookup_paths.R

… with get_locality_data in add_keep_population_flag

…th in fill_geographies arguments

…process_lookup_postcode

LucyEmma22 · 2026-06-02T08:43:25Z

+                                     ),
+                                     BYOC_MODE) {
+  # TODO: Check arguments - do get_datazone_pop_data and get_locality_data just need BYOC_MODE?
+


Do get_datazone_pop_data and get_locality_data need all arguments to be written or is BYOC_MODE sufficient? I.e.

add_keep_population_flag <- function(individual_file, year, pop_estimates = get_datazone_pop_data(BYOC_MODE = BYOC_MODE), locality_data = get_locality_data(BYOC_MODE = BYOC_MODE), BYOC_MODE) {

process_lookup_sc_demographics() in #1313 just uses the BYOC_MODE argument for spd_data, uk_postcode_data and ch_name_lookup.

LucyEmma22 · 2026-06-02T08:43:58Z


-  cli::cli_alert_info("Add keep population function finished at {Sys.time()}")
+  cli::cli_alert_info("Add keep population function finished at {Sys.time()}") # TODO: Is this being kept or changed with a logger_utils function?



Is log message being kept or changed with a logger_utils function?

Happy for us to change it to a logger message. When I made the change of our logs to logger, I changed some of the cli:: alerts to logger since some of them sometimes don't even print in the console so I'm happy for us to change it to logger::log_info instead.

LucyEmma22 · 2026-06-02T08:44:37Z


-  cli::cli_alert_info("Fill geographies function finished at {Sys.time()}")
+  cli::cli_alert_info("Fill geographies function finished at {Sys.time()}") # TODO: Is this being kept or changed with a logger_utils function?



Same comment as above.

Happy for us to change it to a logger message. When I made the change of our logs to logger, I changed some of the cli:: alerts to logger since some of them sometimes don't even print in the console so I'm happy for us to change it to logger::log_info instead.

LucyEmma22 · 2026-06-02T08:48:47Z

+                              BYOC_MODE) {
+  if (isTRUE(BYOC_MODE)) {
+    log_slf_event(stage = "read", status = "start", type = "HSCP Localities Lookup", year = "all") # TODO: Check whether to add hscp_locality to log_slf_event mapping list
+


Should we add add hscp_locality / simd / datazone_pop to log_slf_event mapping list?

"hscp_locality" ~ "HSCP Localities Lookup", "simd" ~ "SIMD Lookup", "datazone_pop" ~ "DataZone Population Lookup"

LucyEmma22 · 2026-06-02T08:51:21Z

+      dplyr::select(
+        locality = "hscp_locality",
+        tidyselect::matches("datazone\\d{4}$")
+      ) %>% # TODO: Check whether we need to select columns


Do we need to select columns when data is read in (either from Denodo or disk)?Columns could be selected when the data is read in or when it is used.

NOTE: For get_simd_data() the column names within the file will change when a new version of the SIMD is released.

Comment also applies to get_simd_data() and get_datazone_pop_data().

LucyEmma22 · 2026-06-02T08:52:29Z

+
+    log_slf_event(stage = "read", status = "complete", type = "HSCP Localities Lookup", year = "all") # TODO: Check whether to add hscp_locality to log_slf_event mapping list
+  } else { # TODO: Check logic - are we reading the local file when BYOC_MODE = FALSE or are we still reading from Denodo?
+


Are we reading the local file when BYOC_MODE = FALSE or are we still reading from Denodo? read_extract_XXX reads from denodo when BYOC_MODE = FALSE but get_uk_postcode_data and get_spd_data read from disk when BYOC_MODE = FALSE.

Comment also applies to get_simd_data() and get_datazone_pop_data().

#1313 (comment)

LucyEmma22 · 2026-06-02T08:55:14Z

+#' @family lookup files
+get_locality_data <- function(denodo_connect = get_denodo_connection(BYOC_MODE = BYOC_MODE),
+                              file_path = get_locality_path(),
+                              BYOC_MODE) {


Do we need file_path = get_locality_path() and denodo_connect = get_denodo_connection(BYOC_MODE = BYOC_MODE) as arguments? denodo_connect is only used if BYOC_MODE = TRUE and file_path is only used if BYOC_MODE = FALSE so we could just have get_locality_data <- function(BYOC_MODE) then define file_path and denodo_connect within the ifelse statement? They always take the same value so is there a benefit to having them as arguments? There may be other reasons to keep them that I have not thought about.

Comment also applies to get_simd_data() and get_datazone_pop_data().

LucyEmma22 · 2026-06-02T08:57:07Z

-    ...
-  )
+get_slf_postcode_path <- function(update = latest_update(), BYOC_MODE, ...) { # TODO: Check whether to keep the update argument
+


Should the update = latest_update() argument be kept? I seem to remember it being removed from somewhere else.

LucyEmma22 · 2026-06-02T09:15:00Z

                                    write_to_disk = TRUE) {
+  # TODO: Check arguments - do get_spd_data, simd_data and get_locality_data just need BYOC_MODE?
+  #       Alternatively we could have no default and just call data in targets (i.e. same as process_extract_XXX).
+


Like add_keep_population_flag, do get_spd_data, get_simd_data and get_locality_data need all arguments to be written or is BYOC_MODE sufficient? I.e.

process_lookup_postcode <- function(spd_data = get_spd_data(BYOC_MODE = BYOC_MODE), simd_data = get_simd_data(BYOC_MODE = BYOC_MODE), locality_data = get_locality_data(BYOC_MODE = BYOC_MODE), BYOC_MODE = FALSE, run_id = NA, run_date_time = NA, write_to_disk = TRUE) {

Alternatively we could have no default and just call data in targets (i.e. same as process_extract_XXX). I.e.

process_lookup_postcode <- function(spd_data, simd_data, locality_data,, BYOC_MODE = FALSE, run_id = NA, run_date_time = NA, write_to_disk = TRUE) {

LucyEmma22 · 2026-06-02T10:33:17Z

+    dplyr::mutate(
+      run_id = run_id,
+      run_date_time = run_date_time
    )


Is this needed?

LucyEmma22 · 2026-06-02T10:59:13Z

get_spd_data() needs to be added as a target.

LucyEmma22 · 2026-06-02T10:59:41Z

get_spd_data() needs to be added as a target.

OluwatobiOni · 2026-06-05T13:02:16Z

+  ),
  slf_gpprac_lookup = read_file(
-    get_slf_gpprac_path(),
+    get_slf_gpprac_path(BYOC_MODE = BYOC_MODE),


Thanks Lucy! No need for me to do this on my branch then.

Merge order: spd -> simd_locality > gpprac_lookup

OluwatobiOni · 2026-06-05T13:09:40Z

-      )
-    )
+    fill_postcode_geogs(slf_pc_lookup) %>%
+    fill_gpprac_geographies(slf_gpprac_lookup)


Thanks Lucy! No need for me to do this on my branch then.

LucyEmma22 and others added 10 commits June 1, 2026 17:35

Add get_locality_data(), get_simd_data() and get_datazone_pop_data() …

3c2fbce

…functions to get_lookup_paths.R

Refactor get_slf_postcode_path

a924b32

Replace get_pop_path with get_datazone_pop_data and get_locality_path…

559409f

… with get_locality_data in add_keep_population_flag

Add BYOC_MODE argument to get_slf_postcode_path and get_slf_gpprac_pa…

3520c0d

…th in fill_geographies arguments

Refactor process_lookup_postcode

b81fc70

Replace locality_path and simd_path in _targets and add arguments to …

59269ea

…process_lookup_postcode

Add locality_data, simd_data and source_pc_lookup to dummy_targets

2ebe66f

Add postcode_lookup to run_sdl

6e3f1e5

Update documentation

762b09e

Style code

bf2060c

LucyEmma22 commented Jun 2, 2026

View reviewed changes

Comment thread SDL_process/dummy_targets.R

LucyEmma22 Jun 2, 2026

Copy link
Copy Markdown

Collaborator Author

Choose a reason for hiding this comment

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

get_spd_data() needs to be added as a target.

LucyEmma22 commented Jun 2, 2026

View reviewed changes

Comment thread _targets.R

LucyEmma22 Jun 2, 2026

Copy link
Copy Markdown

Collaborator Author

Choose a reason for hiding this comment

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

get_spd_data() needs to be added as a target.

OluwatobiOni reviewed Jun 5, 2026

View reviewed changes

Jennit07 added ready for PHS testing Preprod data not available labels Jun 9, 2026


		cli::cli_alert_info("Add keep population function finished at {Sys.time()}")
		cli::cli_alert_info("Add keep population function finished at {Sys.time()}") # TODO: Is this being kept or changed with a logger_utils function?


		cli::cli_alert_info("Fill geographies function finished at {Sys.time()}")
		cli::cli_alert_info("Fill geographies function finished at {Sys.time()}") # TODO: Is this being kept or changed with a logger_utils function?


		log_slf_event(stage = "read", status = "complete", type = "HSCP Localities Lookup", year = "all") # TODO: Check whether to add hscp_locality to log_slf_event mapping list
		} else { # TODO: Check logic - are we reading the local file when BYOC_MODE = FALSE or are we still reading from Denodo?

Conversation

LucyEmma22 commented Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LucyEmma22 Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LucyEmma22 Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LucyEmma22 Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LucyEmma22 Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OluwatobiOni Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LucyEmma22 commented Jun 2, 2026 •

edited

Loading

LucyEmma22 Jun 2, 2026 •

edited

Loading

LucyEmma22 Jun 2, 2026 •

edited

Loading

LucyEmma22 Jun 2, 2026 •

edited

Loading

LucyEmma22 Jun 2, 2026 •

edited

Loading

OluwatobiOni Jun 5, 2026 •

edited

Loading