DIFM-Brain / ofpetrial

GNU General Public License v3.0
0 stars 1 forks source link

modify change_rates() so that the modified trial designs can be visualized properly #6

Closed tmieno2 closed 4 months ago

tmieno2 commented 5 months ago

When a new rate is introduced to a trial design using changes_rates(), they appear as blank on the map because it is not part of the factor levels of the original rates.

tmieno2 commented 4 months ago

Stop adding base rate information to rate information. Rather, it can be added when viz(type = "rate").

tmieno2 commented 4 months ago

@brittanikedge, I am considering some consequential changes to prep_rate_info() and viz(). Basically, I will remove equivalent conversion and adding base rate from prep_rate_info() and then do those jobs on the fly at the time of viz(). Given they are not at all relevant in assigning rates, this is cleaner. Also, this solves the problem I mentioned above that happens when rates are changed manually after assign_rates(). I can see that this immediately affects map creation in make_trial_reports(). I did not look at that part of the code very carefully, but it looks very similar to what we have in viz(). Can't you just use viz() instead? Are there any special things you are adding to the map only in the report? Please let me know if there are other complications.

brittanikedge commented 4 months ago

@tmieno2 That all makes sense to me; the base rate and conversion are only used for visualization or reporting purposes. Regarding the viz() and the visualizations in the report, I noticed the same thing while working on another issue. I will use viz() in the report rather than repeating essentially the same code.

tmieno2 commented 4 months ago

Got it. I noticed that I need to modify get_figure_title(). In the code below, When does include_base_rate == FALSE & "tgt_rate_equiv" %notin% rate_cols happen?

  name <- if (include_base_rate == FALSE & "tgt_rate_equiv" %notin% rate_cols) {
    paste0(input_name, " (", unit, "/", land_unit, ") | ", "No base application")
  } else if (include_base_rate == FALSE & "tgt_rate_equiv" %in% rate_cols) {
    paste0(
      input_name, " (", unit, "/", land_unit, ") | ",
      input_type, " Equivalent (", converted_unit, "/", land_unit, ") \n", "No base application"
    )
  } else {
    paste0(
      input_name, " (", unit, "/ha) | ",
      input_type, " Equivalent (", converted_unit, "/", land_unit, ") | ",
      "Total ", input_type, " (", converted_unit, "/", land_unit, ") \n",
      paste0("Base application: ", base_rate_equiv, " (", converted_unit, "/", land_unit, ")")
    )
  }
brittanikedge commented 4 months ago

@tmieno2 Well, it could be seed or any other input that does not have a conversion, such as a biostimulant. That's why I tried to move away from just using seed as the if statement.

brittanikedge commented 4 months ago

"tgt_rate_equiv" %notin% rate_cols would happen when there was no conversion found for the given input_name and unit combination.

tmieno2 commented 4 months ago

Okay. Got your intension. But, seed goes to the third category right? Right now we have this in prep_rate_info().

if (input_trial_data$input_name != "seed") {
    tgt_rate_equiv <- convert_rates(input_trial_data$input_name, unit, rates_ls)
  } else {
    tgt_rate_equiv <- tgt_rate_original
  }

So, tgt_rate_equiv is always there at the moment. We probably should create a list of inputs that need conversion and does not and refer to them.

brittanikedge commented 4 months ago

@tmieno2 Well, tgt_rate_original is there, but rate_cols only contains the columns with unique values, so it ignores tgt_rate_equiv when it is the same as tgt_rate_original.

I don't have a strong feeling about how we handle this as there are quite a few options. My main concern was having the visualization figure work when it is a new input_name value. We can't create a list of all input possibilities and whether they should or shouldn't be converted. However, we do have a list of those that can be converted inside the conversion table.

tmieno2 commented 4 months ago

Oh, okay. I misunderstood what the rate_cols lines do. I will actually run that part of the code. But, wether tgt_rate_original is the same as target_rate_equiv is determined completely inside prep_rate() right? Are you future-proofing or am I missing something?

tmieno2 commented 4 months ago

Okay, I don't know how this checks the uniqueness of the various rates. Do you have time to chat tomorrow?

list(data.table(
        tgt_rate_original = unlist(tgt_rate_original),
        tgt_rate_equiv = unlist(tgt_rate_equiv),
        total_equiv = unlist(total_equiv)
      ) %>%
        data.frame(.) %>%
        .[colSums(is.na(.)) == 0]
brittanikedge commented 4 months ago

I think you are looking at an old version. I added an extra line to avoid the problem with the seed trial design so it only keeps the unique columns:

data.table(
        tgt_rate_original = unlist(tgt_rate_original),
        tgt_rate_equiv = unlist(tgt_rate_equiv),
        total_equiv = unlist(total_equiv)
      ) %>%
        data.frame(.) %>%
        .[colSums(is.na(.)) == 0] %>%
        .[!duplicated(as.list(.))] %>%
        colnames(.))

Yes, I am available to meet anytime tomorrow except 10-11 AM. I was adjusting the viz() function based on the current version of prep_rates_info() and thinking about inputs like pivotbio that won't be in the conversion dictionary. However, we should be able to adjust prep_rates_info() in a way that makes the viz() more efficient.

tmieno2 commented 4 months ago

Ah, got it. Okay, how about 11am?

brittanikedge commented 4 months ago

Perfect! I will send a Teams invitation.