cynkra / munch

Functions for working with the historicized list of communes of Switzerland.
https://munch.cynkra.com
6 stars 2 forks source link

WIP: Fix R-devel failure #40

Closed krlmlr closed 3 years ago

krlmlr commented 3 years ago

Start with consistent behavior on R 4.0 and R devel, now failing everywhere.

Closes #39.

codecov[bot] commented 3 years ago

Codecov Report

Merging #40 (a46f85b) into main (f900626) will decrease coverage by 0.00%. The diff coverage is 38.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #40      +/-   ##
==========================================
- Coverage   37.73%   37.73%   -0.01%     
==========================================
  Files          16       16              
  Lines         628      636       +8     
==========================================
+ Hits          237      240       +3     
- Misses        391      396       +5     
Impacted Files Coverage Δ
R/swc_get_mapping.R 70.83% <21.42%> (-1.36%) :arrow_down:
R/swc_get_mutations.R 94.52% <100.00%> (-1.20%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f900626...a46f85b. Read the comment docs.

TSchiefer commented 3 years ago

It seems to me, that there are just some municipality ids missing in the call swc_get_mapping(ids_from=ids_from, ids_to=ids_to)

The example works if we don't filter the mids (all.ids <- c(tid(ids_from), tid(ids_to)) in the function swc_get_mutations(mids = NULL, canton = NULL):

  if (!is.null(mids)) {
    municipality_mutations <-
      municipality_mutations %>%
      filter(mId %in% !!mids)
  }

The problem arises in the self-merge() call a little later, where mAbolitionId is matched with mAdmissionId.

I am not sure what the lists ids_from and ids_to mean and why it should be logical that no mids are missing.

Not filtering the mids misses the point of the argument mids in swc_get_mutations though.

But currently, the waiting time is not very long if we don't filter. Shall I just remove the argument and remove the filtering code snippet from above?

TSchiefer commented 3 years ago

Actually, for swc_get_mutations() to properly work with a mids-list, the list needs to contain both the mid_from and mid_to for each mutation. Is that the intention? (it says in the documentation that if the self-merge doesn't find a match, NAs are produced)