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.

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

Use tar_map() to reduce duplicated code #374

Open
Robinlovelace opened this issue Jan 3, 2024 · 4 comments
Open

Use tar_map() to reduce duplicated code #374

Robinlovelace opened this issue Jan 3, 2024 · 4 comments
Assignees

Comments

@Robinlovelace
Copy link
Contributor

Robinlovelace commented Jan 3, 2024

From the targets manual:

Sometimes, a pipeline contains more targets than a user can comfortably type by hand. For projects with hundreds of targets, branching can make the _targets.R file more concise and easier to read and maintain.

Just saw this tip from @andrewheiss (thanks for sharing):

Finally figured out how to dynamically make {targets} targets through metaprogramming (https://books.ropensci.org/targets/static.html#metaprogramming) and it's SO NICE to have nice clean target names that are automatically generated #rstats

Source: https://fosstodon.org/@[email protected]/111689215513772257

Currently our code has a lot of bits like this:

npt/_targets.R

Lines 322 to 328 in f1ae9eb

tar_target(r_commute_fastest, {
rs_commute_fastest[[1]]$routes
}),
tar_target(r_commute_quietest, {
rs_commute_quietest[[1]]$routes
}),

An iterator like tar_map() could compress those 3 targets into a single function call, reducing complexity and easing readability and maintenance.

@Robinlovelace
Copy link
Contributor Author

Robinlovelace commented Jan 3, 2024

I tested this on a modified version from the documentation:

  list(
    tarchetypes::tar_map(
      list(a = c(12, 34, 1), b = c(45, 78, 9)),
      targets::tar_target(x, a + b),
      targets::tar_target(y, x + a, pattern = map(x))
    )
  )

And the results show that the 'target factory' is accurate:

image

This can be part of a wider refactor discussed, will do some tests first but I think we can greatly simplify things as per #343 but taking it to the next level.

@Robinlovelace Robinlovelace self-assigned this Jan 3, 2024
atumscott added a commit that referenced this issue Jan 25, 2024
@Robinlovelace
Copy link
Contributor Author

Taking a look at this now.

Robinlovelace added a commit that referenced this issue Feb 14, 2024
Robinlovelace added a commit that referenced this issue Feb 14, 2024
FYI @mem48 this is how you iterate with targets
Robinlovelace added a commit that referenced this issue Feb 15, 2024
@Robinlovelace Robinlovelace linked a pull request Feb 15, 2024 that will close this issue
@Robinlovelace
Copy link
Contributor Author

Heads-up @mem48 and @wangzhao0217 these are the regions I'm planning to use to geographically batch the build process, enabling all routes to be held in a single list:

image

@Robinlovelace
Copy link
Contributor Author

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant