Add ability to modify record options

[barrettk]

• allow for modification of option values/presence
• allow for adding a new options
• limit to $EST, $TABLE, and $DATA records for now

[barrettk]

@kyleam Just as a heads up... I know we can access table columns via get_record_option(record, name = "list1"), but I may implement this a slightly different way initially (for the new function). We can talk more when it's ready, but just letting you know I had a specific reasoning for not going that route.

Background

Edit: here is what I was thinking. I personally like retaining the functionality of allowing modifications (not just adding). This is inline with what @seth127 and I discussed as well, though I do see your point about it being unnecessary/the intent of the package.

If we are ok with this functionality, I implemented the following for updating table columns (not committed). FWIW (mostly for Seth), table columns are stored as a single string (all concatenated), so they have to be updated differently from the others. To clarify:

> record$values[[3]]$name
[1] "list1"
> record$values[[3]]$name_raw
NULL
> record$values[[3]]$value
[1] "ID TIME PRED RES WRES CPRED CWRES EPRED ERES EWRES"

Implementation

Original record

> record
$TABLE ID TIME PRED RES WRES CPRED CWRES EPRED ERES EWRES NOAPPEND
       ONEHEADER FILE=example1.TAB NOPRINT

Modifications

# modify positional option
modify_record_opt(record, name = "PRED2", value = TRUE)
> record
$TABLE ID TIME PRED RES WRES CPRED CWRES EPRED ERES EWRES PRED2 NOAPPEND
       ONEHEADER FILE=example1.TAB NOPRINT
       

# This looks similar to how we modify flags:
modify_record_opt(record, name = "NOPRINT", value = FALSE)
> record
$TABLE ID TIME PRED RES WRES CPRED CWRES EPRED ERES EWRES PRED2 NOAPPEND
       ONEHEADER FILE=example1.TAB 

I see an issue with this. Mainly, that it treats positional options similar to flags (looks like the value is logical when in reality it's actually the character string of "PRED2". I imagine this will be a concern and not something you will want to go through with for consistency purposes. This was the only method I could come up with that allowed for the removal of column names as well though (without adding extra arguments).

If we're adamant about this function only having the ability to add options, this is a moot point. Anyways, mainly wanted the three of us to be on the same page with what the function should handle. The above discussion is only worth having if we're considering opening the function up to allowing modifications as well.

[seth127]

Interesting.

Part 1: add/modify/remove

Off the top, I feel pretty strongly that any helper we should be able to add, modify, or remove the relevant options. I could potentially see these being different functions (e.g. add_est_opt(), remove_est_opt(), etc.) but that feels a bit overboard to me. I could be convinced though, if there's good reason to approach it that way.

Part 2: just make them pass the whole string?

Focusing back on this specific case (the columns in the table record), I may not be understanding this quite right, but if the issue is just that all columns are specified/parsed as a single option... what if we just make the users do that too? So the interface would be kind of like:

modify_record_opt(record, "columns", vector_of_column_names)

My thinking:

1. The order of columns likely matters to the users, and I'm struggling to imagine any sane interface that let's them say "insert this column at this index..."
2. It doesn't seem unreasonable that they would have the vector of desired column names in hand, such that they could pass it back in. If nothing else, they could extract what's currently there, modify the vector, and then pass it back.
3. If this works, then you could sidestep this whole thing by just treating it like any other table option.

Does that make sense or am I totally missing some of the nuance?

Part 3: separate helpers per opt

My other thought is, this is part of why I was thinking about implementing specific helpers for each type of record. For example, something like this design:

#' private
modify_opt_basic_impl <- function(...) {
  # does the modification for all records that work for the basic case
  # i.e. $DATA, $ESTIMATION, etc.
}

#' @export
modify_estimation_opt <- function(...) {
  # check the the passed record is "estimation" type
  modify_opt_basic_impl(...)
}

#' @export
modify_data_opt <- function(...) {
  # check the the passed record is "data" type
  modify_opt_basic_impl(...)
}

Then modify_table_opt() could easily have special handling (possibly an extra arg or two) for the "columns" option that is a special case.

That said... I'm trying to stub out that interface and having a hard time envisioning it. So maybe that's a sign that's it a bad idea.

[kyleam]

[ This is jumping into discussion of how to handle table listN options, so I'll leave comments on higher-level design be for now. ]

@barrettk:

I see an issue with this. Mainly, that it treats positional options similar to flags (looks like the value is logical when in reality it's actually the character string of "PRED2". I imagine this will be a concern and not something you will want to go through with for consistency purposes.

As you mention, nmrec does not parse within listN values. The level that is available to set at, then, is the entire string. If the setter tries to handle unparsed components, it's overreaching in my view.

nmrec provides the user a way to get that string, handling a lot of the complication, but, at least with what's currently supported, everything else is left to them. That's going to be the case with a lot of record details.

[kyleam]

I think this helper should focus on options defined via *_option_types and *_option_names variables. These are the "standard" options mentioned by gh-4 and cover the majority of options. The remaining ones are for an assortment of scenarios that in my opinion are reasonable to not cover with this convenience helper.

The interface could then be

set_record_option(record, name, value)

Implementation details:

• look up name and type via corresponding *_option_names and *_option_types variables

  This logic is already implemented in get_record_option(), so it makes sense to extract that to a shared helper.

  Error if the option isn't known. Docs should explain that this covers "standard" flag and value options.

• if option is present more than once, signal an error

• if option is present once, replace it with the new option

• if option isn't present, add it on its own line at the end (with some fixed indentation)

• treat value as described for flag and value options in the option docs

• I'd vote to return NULL invisibly to clearly communicate that this helper is modifying record

[seth127]

Responding to @kyleam 's comments...

nmrec does not parse within listN values. The level that is available to set at, then, is the entire string...

I think this is essentially the same as I was proposing in "Part 2" of my comment above, right?

I think this helper should focus on options defined via *_option_types and *_option_names variables... Implementation details...

This comment all makes sense to me. @barrettk read through that a few times and see if you have any outstanding questions. I think that

Another comment for future direction...

If the setter tries to handle unparsed components, it's overreaching in my view.

That definitely makes sense to me. To be clear though, the intention is likely to parse (at least some of) those listN options at some point in the future, right? And then at that point, I would think we could potentially revisit adding some kind of setter for those as well (either modifying this one, or adding dedicated ones). Am I understanding that right?

[kyleam]

I think this is essentially the same as I was proposing in "Part 2" of my comment above, right?

Yes. (Sorry for not making that clear; my reply was meant as a direct response to @barrettk's question.)

To be clear though, the intention is likely to parse (at least some of) those listN options at some point in the future, right? And then at that point, I would think we could potentially revisit adding some kind of setter for those as well (either modifying this one, or adding dedicated ones).

If we need them (and definitely if we need to set them), I think we should parse them. But, as with records themselves, there are lots of stuff that we don't parse, and I don't think nmrec should state that its goal is to eventually support everything (edit: by that I mean, parsing everything down to the level that every possible user would possibly want).

If we do add support for parsing within table list values, I agree that it makes sense to revisit the setter, though I don't think we necessarily need to add one. If we do add this setter functionality, my current view is that we shouldn't tack that onto this setter as it would complicate the interface and implementation of the common case of "standard" options, but we can of course cross that bridge when/if we get to it.

[barrettk]

@kyleam @seth127

Based on the later suggestions, Im wondering if the code I committed on Friday could be worth a more broad review.

• There is some shared code between get_record_option and modify_record_opt, but I removed some of the additional handling we either didnt want or didnt need (and this).
• It does work with list1 type elements, but causes a concern highlighted in the failing test (any unrecognized value (such as a SCIENTIFITC positional option) is treated as a new column). I therefore am thinking we dont want to handle columns at the moment like Kyle suggested. If anything this should probably be a separate function, so that the user is well aware any string they choose is treated as a column, and not some other argument they expect to be in a particular location (for NONMEM to parse).

Implementation details:

look up name and type via corresponding *_option_names and *_option_types variables . This logic is already implemented in get_record_option(), so it makes sense to extract that to a shared helper.

This is done, but it didnt seem worth making a shared helper, given how little of it I used (plus I modified it).

Error if the option isn't known. Docs should explain that this covers "standard" flag and value options.

This is done, though I still have to remove the "pos" option (kept it in while developing - there seems to be a potential reason to keep this for some new positional options (like the scientific option I mentioned, but it seems these have to be part of a list/environment for nmrec to view it as a valid option with an expected location).

if option is present more than once, signal an error

Hooked up

if option is present once, replace it with the new option

Hooked up

if option isn't present, add it on its own line at the end (with some fixed indentation)

Gets appended to the end, but not as a new line. Easy change if that's what we want.

treat value as described for flag and value options in the option docs

That's how it currently works

I'd vote to return NULL invisibly to clearly communicate that this helper is modifying record

Will do this and add documentation.

@seth127 If we proceed with columns not being able to be adjusted using this function, I dont think it makes sense to be able to use a vector of options. A) for clarity (treat it as similar to get_record_option), and B) would complicate the function (if some were option_value and others were option_flag).

[seth127]

@kyleam I just had a conversation with @barrettk . He is going to push a few things to streamline this helper and, I'm pretty sure, coalesce to what you outline here.

A few notes:

• He had started to build in some special handling for positional arguments, specifically the $TABLE columns. He is going to remove that and make a new issue with his thoughts on a possible helper to deal with those directly.
• He was considering an opt_type arg as well, but I propose that we can infer that based on whether the user passes a logical (we infer "flag"), a character/numeric (we infer "value"), or a NULL (we remove the specified option).

[kyleam]

He was considering an opt_type arg as well, but I propose that we can infer that based on whether the user passes a logical (we infer "flag")

Yes, I agree that there should not be a type argument (as I proposed above, I'd like this to be set_record_option(record, name, value)).

You can know the type for sure via the *_option_types variables I mentioned above.

[kyleam]

He is going to remove that and make a new issue with his thoughts on a possible helper to deal with those directly.

Okay, though I think it's worth stepping back and assessing actual use cases we have.

He is going to push a few things to streamline this helper and, I'm pretty sure, coalesce to what you outline here.

Okay. @barrettk Let me know when it's ready to take an initial look. (I'll likely meet with you to clean up the history and tweak other minor things, but we can leave that until the main code changes from the reviews settle.)

[barrettk]

@kyleam just marked you to review. The one thing I didn't implement yet was the returning of NULL. Figured it might be good to have a first pass review of the code before we assessed that.

In terms of the new helper function - I brought up to @seth127 that the only reason I could see for adding it, is if bbr or bbr.bayes would need to add new columns we created under the hood or something of that nature (such as a new NUM column or something). It was suggested that this was a possibility, so that is the primary motivation for this function. At the moment, user's can still use this function for amending column names using the following logic:

  ctl <- parse_ctl(get("bayes1", envir = nmrec_examples))
  recs <- select_records(ctl, "table")
  rec <- recs[[1]]

  starting_cols <- get_record_option(rec, "list1")$format()
  > starting_cols
[1] "ID TIME PRED RES WRES CPRED CWRES EPRED ERES EWRES"



  set_record_option(rec, name = "list1", value = paste(starting_cols, "PRED2"))
  > rec
$TABLE ID TIME PRED RES WRES CPRED CWRES EPRED ERES EWRES PRED2 NOAPPEND
       ONEHEADER FILE=example1.TAB NOPRINT
  

As an aside, I imagine we will want some @examples (given that "list1" isnt that obvious of a name)

[kyleam]

Thanks @barrettk. Here's my initial round of comments. (I didn't look too closely at the tests yet, but noticed you did a nice job of covering the different cases.)

[kyleam]

@barrettk Thanks for all the work on this.

Most of my later review happened on live sessions, but, making one final pass, this looks great to me.

[kyleam]

Thanks for thinking through so many cases. Looks great.

[kyleam]

I like this approach.

It could do some extra clean up to avoid extra spaces when removing an option, but I don't think it's worth the effort.

[seth127]

Great stuff all around. Thanks y'all.

[seth127]

@kyleam @barrettk it seems like this is loitering and it may be that we all think we're waiting on someone else... is that right?

Either way, my questions at this point are:

1. Is there anything else that needs to happen here, or can this be merged?
2. Should we make a new nmrec release once it merges, or do we want to wait for anything else to land?

[kyleam]

I've approved it. From my POV, it's ready to merge.

For the release: if it'd be helpful for a particular case (pretty much if @barrettk would like it for work in bbr), sure. If he doesn't expect to use it immediately or is fine installing from the repo, we can also wait and see what else we get to soon (in particular, the higher-level parameter helpers).

[barrettk]

@kyleam @seth127 If we dont foresee any notable feature additions in the near future, I would say a release would be preferable just so we can actually merge the test-threads PR once this is hooked up (i.e. adding nmrec to pkgr and as a dependency in the Description file). If there are upcoming features within the next week or two, im fine to wait.

[kyleam]

actually merge the test-threads PR once this is hooked up

The release can be made quickly, and the final changes to get it hooked up are trivial (assuming we're okay with bbr not having a dependency on MPN). So, let's wait a bit to give a little runway for other things. The minute you're nearing a "ready to review" state with test threads, let me know, I'll prepare the release pr for you to approve.

[seth127]

That all sounds good to me.

see what else we get to soon (in particular, the higher-level parameter helpers)

That was the only other thing I had in mind too, but we can see how things shake out over the next couple weeks. Thanks both.

[seth127]

FYI - I just added this issue in bbr related to this. We can talk about which of those we think it's worth tackling in the near future. I know test_threads() is near the top of the list.