Delete climate_match function once it is completely up and running in its own package ClimateCastR

[soriadelva]

As discussed with Tim, the functionality currently available in the climate_match function would better be stored in its own package (called ClimateCastR) and divided into multiple, smaller functions, instead of one big function. We are therefore planning to construct this package during the coming months and move the functionality there. @damianooldoni once everything is up and running we would remove this function from the trias package to avoid confusion. What would you need from our side to make this work fluently?

[damianooldoni]

Nice to hear that! 👍 From the trias side, not really much is needed. Once the climate match functionality built in trias can be reproduced using ClimateCastR, please let me know in this issue. I need to:

1. make the trias function deprecated. It's not really nice for users to have a function suddenly removed 😄
2. Refactor the function so that it actually wraps the code in ClimateCastR
3. After six months or so, remove deprecation but arise error. Something like: "The function 'climate_match()' has been removed. Use climateCastR functionx()instead. For more documentation, check the package documentation 'https://x'".

Just a note: IMO I would name the new package climateCastR instead of ClimateCastR.

[soriadelva]

Nice to hear that! 👍 From the trias side, not really much is needed. Once the climate match functionality built in trias can be reproduced using ClimateCastR, please let me know in this issue. I need to:

1. make the trias function deprecated. It's not really nice for users to have a function suddenly removed 😄
2. Refactor the function so that it actually wraps the code in ClimateCastR
3. After six months or so, remove deprecation but arise error. Something like: "The function 'climate_match()' has been removed. Use climateCastR functionx()instead. For more documentation, check the package documentation 'https://x'".

Just a note: IMO I would name the new package climateCastR instead of ClimateCastR.

Great, we'll keep you posted! 👍 and we'll think about the name 😉

[PietrH]

There are dutch words in objects in climate_match.R, that should be avoided as it hinders outside contributions.

eg. n_totaal

[PietrH]

There are a few magical objects in climate_match.R, such as b, when skimming code this makes it more difficult to follow. I'd also add as a minor note, that two letter or 3 letter object symbols are difficult to interpret when skimming, if at all possible, it would be great if you could rename these objects in scope to be more descriptive.

As a general rule, the further away an object is used from it's initialisation, the longer it's name should be.

example:

  cm <- data.frame()

  for (b in unique(future_climate$scenario)) {
    future_scenario <- future_climate %>% 
      dplyr::filter(.data$scenario == b)

    cm_int <- data_overlay_unfiltered %>% 
      dplyr::filter(
        .data$Classification %in% future_scenario$Classification
      ) %>% 
      dplyr::mutate(scenario = b)

    if (nrow(cm) == 0) {
      cm <- cm_int
    } else {
      cm <- rbind(cm, cm_int)
    }
  }