Creating Additional Column for Starring

[rymarinelli]

closes #474

[rymarinelli]

@kylebaron @seth127 Do you think this should be ordinal in nature and should be integrated into the filtering from #477 ?

[seth127]

Do you think this should be ordinal in nature

No, I think the intent is for it to be boolean so that users can easily filter with:

run_log() %>% filter(starred)

[rymarinelli]

@kyleam Do you think could look at workflow used in this PR? Right now I am using helper functions to add boolean data to rows in the logs. I use add_star, remove_star, and replace_star to manipulate the models. But I am not sure where to call reconcile_yaml. Do you think it is alright to leave that to the end user to know they need to use that function to update the YAML? Also, I wanted your thoughts if I needed additional tests for copy_model_field and modify_model_field

[kyleam]

But I am not sure where to call reconcile_yaml

Shouldn't the *_star wrappers behave in the same way as any of the other modify_model_field-based wrappers? Those let modify_model_field call check_yaml_in_sync and error if the in-memory and disk values aren't in sync, pointing the user to reconcile_yaml.

[seth127]

Shouldn't the *_star wrappers behave in the same way as any of the other modify_model_field-based wrappers?

Short answer: yes

Also, I glanced at this earlier and had this question: what does replace_star() do? In the case of replace_tag() it's "I want to replace this tag with this new tag" but I think for stars it's either you have it or you don't, so I think add_star() and remove_star() should cover it.

[kyleam]

@seth127

Also, I glanced at this earlier and had this question: what does replace_star() do? [...]

Ah, I was just wondering the same thing.

[rymarinelli]

@seth127 I used replace_star to try and follow the naming conventions. It seemed most of the functions were named replace_{field_name} Should I not follow this then?

[kyleam]

Also, I wanted your thoughts if I needed additional tests for copy_model_field and modify_model_field

I think modify_model_field is tested well enough by the existing add_star and remove_star tests. For copy_model_field, either adding a new test to test-copy-model-from.R or modifying copy_from_model options work [BBR-CMF-002] sounds like a good idea to me.

[seth127]

I used replacestar to try and follow the naming conventions. It seemed most of the functions were named replace{field_name} Should I not follow this then?

Generally yes, that's the right move. And you do that with add_star() and remove_star(). I just don't think replace_star() is necessary or really makes sense. What would you be replacing it with?

[rymarinelli]

I used replacestar to try and follow the naming conventions. It seemed most of the functions were named replace{field_name} Should I not follow this then?

Generally yes, that's the right move. And you do that with add_star() and remove_star(). I just don't think replace_star() is necessary or really makes sense. What would you be replacing it with?

I was thinking that it would be replacing the state of .starred.

[seth127]

I think we should keep it add_star() and remove_star() for consistency of user interface.

I also think remove_star() doesn't need to use modify_model_field() internally, because it's the only function we have that actually blows away a model field instead of modifying it. Changing modify_model_field() to make it work for remove_star() feels unnecessary. I would just implement remove_star() like this:

remove_star <- function(.mod) {

  # update .mod with any changes from yaml on disk
  check_yaml_in_sync(.mod)

  .mod[[YAML_STAR]] <- NULL

  # overwrite the yaml on disk with modified model
  .mod <- save_model_yaml(.mod)

  return(.mod)
}

Then the tests can just change to be

test_that("Checking that set_star_status is reflecting boolean data correctly YAML[BBR-MMF-023]", {
  temp_mod_path <- create_temp_model()
  new_mod <- read_model(temp_mod_path)
  expect_null(new_mod$star)

  new_mod <- add_star(new_mod)
  expect_true(new_mod$star)

  new_mod <- remove_star(new_mod)
  expect_null(new_mod$star)

})

[kyleam]

@seth127:

I also think remove_star() doesn't need to use modify_model_field() internally [...]

Ah, interesting point.

Doesn't that pretty much apply to add_star() too? It is setting a value, but that logic looks much closer to remove_star() than modify_model_field(). So I guess I'm suggesting a light internal helper called by remove_star() and add_star() that essentially does what you posted but takes the desired state as an argument. A nice advantage of that is that we could return modify_model_field() back to its state before this PR, and avoid adding the .bool_value parameter.

Then the tests can just change to be [...]

It looks like you forgot to remove the second argument (value).

[rymarinelli]

@seth127:

I also think remove_star() doesn't need to use modify_model_field() internally [...]

Ah, interesting point.

Doesn't that pretty much apply to add_star() too? It is setting a value, but that logic looks much closer to remove_star() than modify_model_field(). So I guess I'm suggesting a light internal helper called by remove_star() and add_star() that essentially does what you posted but takes the desired state as an argument. A nice advantage of that is that we could return modify_model_field() back to its state before this PR, and avoid adding the .bool_value parameter.

Then the tests can just change to be [...]

It looks like you forgot to remove the second argument (value).

I think the .boolean is needed for new_model. Otherwise, the only expected input for the constructor are characters.

[kyleam]

@rymarinelli:

Otherwise, the only expected input for the constructor are characters.

Sorry, I'm not following. Which constructor?

[seth127]

Responses to some comments above

---

So I guess I'm suggesting a light internal helper called by remove_star() and add_star() that essentially does what you posted but takes the desired state as an argument.

I could see this approach being good. And I do think it should be internal. Something like this?

#' Adds or removes a boolean attribute
#' @keywords internal
modify_model_field_lgl <- function(.mod, .field, .action = c("add", "remove")) {

  # update .mod with any changes from yaml on disk
  check_yaml_in_sync(.mod)

  # check action and modify model
  .action <- match.arg(.action)
  if (.action == "add") {
    .mod[[.field]] <- TRUE
  }
  if (.action == "remove") {
    .mod[[.field]] <- NULL
  }

  # overwrite the yaml on disk with modified model
  .mod <- save_model_yaml(.mod)

  return(.mod)
}

And then the two we export would just be:

add_star <- function(.mod) {
  modify_model_field_lgl(.mod, YAML_STAR, "add")  
}

remove_star <- function(.mod) {
  modify_model_field_lgl(.mod, YAML_STAR, "remove")  
}

I kinda love that actually. @rymarinelli and I discussed something like this earlier and I said "don't bother" but I think I've changed my tune. I'm not sure if we'll add other boolean attributes or not, but if we do, this sure makes it easy.

---

A nice advantage of that is that we could return modify_model_field() back to its state before this PR, and avoid adding the .bool_value parameter.

This is also a good point, I think I'd support this. Do you follow @rymarinelli ? Basically modify_model_field() can revert to how it currently looks on develop because we won't be calling it at all with these new functions.

---

I think the .boolean is needed for new_model. Otherwise, the only expected input for the constructor are characters.

I think you're talking about here but maybe I'm wrong. Either way, we talked earlier about switching that to something like

if (isTRUE(.star))     .mod <- add_star(.mod)

---

It looks like you forgot to remove the second argument (value). in the test code I wrote above

This is true. I changed it in the comment above too, but now it should look like

test_that("Checking that set_star_status is reflecting boolean data correctly YAML [BBR-MMF-023]", {
  temp_mod_path <- create_temp_model()
  new_mod <- read_model(temp_mod_path)
  expect_null(new_mod$star)

  new_mod <- add_star(new_mod)
  expect_true(new_mod$star)

  new_mod <- remove_star(new_mod)
  expect_null(new_mod$star)
})

[rymarinelli]

@rymarinelli:

Otherwise, the only expected input for the constructor are characters.

Sorry, I'm not following. Which constructor?

I mostly put the boolean parameter to ensure that boolean input would be passed into .star. I think the function expects character data otherwise.

new_model <- function( .path, .description = NULL, .based_on = NULL, .tags = NULL, .bbi_args = NULL, .overwrite = FALSE, .star = NULL, .model_type = c("nonmem") )

[seth127]

@kyleam I went through this and pair programmed with @rymarinelli to clean a few things up. This should be good to go, but take a quick look to see if any of the changes you requested are still outstanding. Thanks.