Follow-up on feedback from CAA

[archaeothommy]

Function for bdl_strategy that replaces negative values in selected columns with a uniform value (e.g. NA or 0).

Follow-up on feedback gathered at the CAA (#43)

[archaeothommy]

Where I am stuck:

• How to make it work across multiple columns? If I understand the code correctly, the checks are performed per column.
• The value to be compared to has to be a unit. How to assign the appropriate unit?
• Because the comparison value has to be a unit, comparison does not work any more if multiple columns are selected.

[nevrome]

Our old code is not compatible with this kind of strategy. The critical bit is this function in ASTR_colname_parser.R:

apply_bdl_strategy <- function(x, colname, bdl, bdl_strategy) {
  bdl_values <- which(grepl(paste(bdl, collapse = "|"), x, perl = FALSE))
  x[bdl_values] <- bdl_strategy()
  return(x)
}

At the moment the bdl strategy can essentially only be a static function - it does not have any inputs. We forsaw that eventually we could need other strategies so we laid the foundation for more flexibility, but we never fleshed it out. The comment above apply_bdl_strategy can be understood in this context:

# colname only an argument in case we want to implement more specific handling
# eventually

So how do we go beyond this? I see multiple possible solutions. The easiest would probably be to break the current interface and merge bdl and bdl_strategy into one argument. This new argument bdl could just take one function that essentially replaces apply_bdl_strategy. It would be be called where we currently call apply_bdl_strategy and take similar input arguments. With this the bdl strategies we have now could look something like this (haven't tested any of that):

bdl_strategy_default(x, ...) {
  bdl_strings <- c("b.d.", "bd", "b.d.l.", "bdl", "<LOD", "<")
  bdl_indices <- which(grepl(paste(bdl_strings, collapse = "|"), x, perl = FALSE))
  x[bdl_indices] <- NA_character_
  return(x)
}

bdl_strategy_negative(x, ...) {
  y <- suppressWarnings(as.numeric(x))
  bdl_indices <- which(y < 0)
  x[bdl_indices] <- NA_character_
  return(x)
}

I think this would work 🤔. It is also not too complicated; even easier than what we have now. Of course it's a bit tedious to write a custom strategy function, but if we include these two examples then it should be reasonably clear how to do so.

If you're OK with this conceptually I can offer to implement it. Maybe you write a quick (currently failing) test for this requirement. Then I would see right away if my implementation behaves as you would like it to.

[nevrome]

Ah - about your questions: I don't understand what you mean here, honestly.
Why would this have to work across columns? It's an operation that can be done for each column separately, right? Units don't matter in this stage of the processing. At this point everything is still a string.

Could you explain what you mean? I guess I'm just missing something.

[archaeothommy]

Thank you, this sounds very good! I think merging them into a function would be great. It may look more "frightening" but at the same time more logical to have only one input for bdl. And with pre-made functions and good examples, this should actually not be that frightening.

I agree that they should all have the same structure of input (and of course must have output). I envision here a single documentation page for this family. The functions could look like bdl_strategy_<descriptor>(column_names, replacement_value, input_values) with

• column_name: The columns the action should be performed on
• replacement_values: The value(s) that should be used.
• input_values (optional): string with the values to look for

input_values is optional because we need this for e.g. bdl-labels we have now while it is not needed for e.g. checking if values are negative.

Would it be possible to stack multiple functions, maybe something like this?

bdl_strategy = function() {
  function1(column_names, replacement_values, input_values)
  function2(column_names, replacement_values)
}

This would allow to e.g. replace common labels for bdl as it is currently the case while handling e.g. negative values.

For clarification to my questions:
I did not thought about that everything is still a string. Comparing a string with a number would create the same error that I did. This can be easily fixed by using a regex to identify negative values.
Yes, the action can still be performed on each column individually as long as I can perform the action for multiple columns at once. Think about e.g. a dataset with compositional data obtained via laser ablation ICP-MS (quantification results in negative values, which are "bdl") combined with isotope data in delta-notation (where negative values must be kept). I want to restrict replacement of negative values to all compositional columns with LA-ICP-MS data.

Thanks a lot for implementing it, that's a great help! I'm very happy to write a test.

[nevrome]

I implemented a draft of what I had in mind. It's not (yet) what you're describing, but your idea could be added easily. My idea was to literally write out the functions in ASTR_bdl_strategies.R and let users write custom ones like this if they need other strategies. What you're describing sounds like function factories that create the functions automatically. That's well compatible with what I have now.

Our pre-built functions and any the users may write can be used in as_ASTR and read_ASTR like this:

bdl_strategy = bdl_strategy_default

Multiple of these can be combined to be applied one after another with function composition:

bdl_strategy = purrr::compose(bdl_strategy_default, bdl_strategy_negative)

Your factories would be used like this, I guess:

bdl_strategy = make_bdl_strategy(column_names, replacement_values, input_values)

make_bdl_strategy() could just live and be documented in ASTR_bdl_strategies.R with the pre-built ones.

[archaeothommy]

This looks all good to me! Combining functions with

bdl_strategy = purrr::compose(bdl_strategy_default, bdl_strategy_negative)

is perfectly fine.

It works if bdl_strategy function are supplied without any arguments. As soon as I wanted to specify a column name, I get the following error:

Error in `purrr::map2()`:
ℹ In index: 11.
ℹ With name: d65Cu_err2SD.
Caused by error in `bdl_strategy_negative()`:
! argument "x" is missing, with no default

Moreover, even when working, it seems to skip the first column (d65Cu) - negative values there are not replaced.