search

insightsengineering / teal.modules.clinical

Provides teal modules for the standard clinical trials outputs
https://insightsengineering.github.io/teal.modules.clinical/
Other
32 stars 17 forks source link

barchart teal module #53

Open cicdguy opened 3 years ago

cicdguy commented 3 years ago

as to per @npaszty

MDIS/stream_doc/um/report_outputs_brg01.html) ## Task Look at max's modules and finalize them we need four modules: - `tm_g_barchart_simple` - `tm_g_barchart` (advanced) - `tm_g_barchart_ae_simple` - `tm_g_barchart_ae` (advance) have the modules reviewed by a SME NEST/docs/hugo/NEST/tlg-catalog/devel/tlg_catalog/graphs/brg01/ Provenance: ``` Creator: waddella ```
cicdguy commented 3 years ago

@nestcicd you can now push to this repo

Provenance: 

Creator: waddella
cicdguy commented 3 years ago

Needs discussion because analysis needs access to unfiltered ADSL dataset and filtered ADAE dataset. We may disable filtering ADSL, only ADAE is allowed to be filtered. Full join is still required and it is needed to identify the rows from ADSL.

Provenance: 

Creator: mordigm
cicdguy commented 3 years ago

See branch 216_brg01. Current screenshot of app, (most important: encoding panel)

user/1311/files/613d7400-63b1-11ea-9a3d-108c23196e81"> Provenance: ``` Creator: mordigm ```
cicdguy commented 3 years ago
NEST/docs/hugo/NEST/tlg-catalog/devel/tlg_catalog/graphs/brg01/ are doing. The interface above was designed based on these observations. user/1311/files/f0e32280-63b1-11ea-816a-e79477337970) Provenance: ``` Creator: mordigm ```
cicdguy commented 3 years ago

@mordigm I am running the tm_g_barchart_simple example in a docker container and I get the following error and no bar chart is displayed. tm_g_barchart_simple

In display Tried to evaluate following code: { plot <- plot + scale_y_continuous(labels = comma, expand = expand_scale(c(0, 0.5)))} Got following error message: object 'comma' not found

Provenance: 

Creator: npaszty
cicdguy commented 3 years ago

@npaszty Updated. Now the interface looks as follows:

user/1311/files/2273f080-647b-11ea-982e-1273c4829d53"> The following points need to be addressed: NEST/docs/hugo/NEST/tlg-catalog/devel/tlg_catalog/graphs/brg01/, you take the mean over all lab values which means that patients with many lab values will be overrepresented. Is this intended? Or should it be: `mean_over_patients(mean_over_measurements_per_patient(AVAL))`? - The ADLB examples (number 5 and 8) do not work yet because the filtering panel on the right applies the filtering before we get it as `ANL`. The denominator should take the unfiltered merged dataset, the nominator should take the filtered merged dataset. @waddella We need to modify teal for this. - Show R Code generation is left aside for now. Be aware that there are small discrepancies between the module and the plots on tlg-catalog. The module adds additional information. Provenance: ``` Creator: mordigm ```
cicdguy commented 3 years ago

@mordigm

  1. The mean is calculated for the lab - let's say CRP - within treatment group.
  2. Okay, will leave to core team
  3. Agreed, should be done at end after the code and interface are finalized

I pulled and re-built the branch this morning but I don't see the same interface as what is in the screen shot above. It's also very difficult to provide feedback if I can't play around with the app and verify the calcs in the visualization. Still getting the evaluation error which I believe is related to show R code related statements. Based on bullet #3 perhaps all the show R code statement should be removed until we complete the review?

Provenance: 

Creator: npaszty
cicdguy commented 3 years ago

@npaszty I don't have any ShowRCode related functionality, are you on the right branch 216_brg01? I suggest we talk today to solve these issues together. Sent you an invite.

Provenance: 

Creator: mordigm
cicdguy commented 3 years ago

@mordigm

I only ran the visualizations and verifications for ADAE.

I think the user guide and table are helpful. Not sure what #5. means in the user guide. table persists when using or not using fill. maybe it's not functional yet.

  1. Bar chart # 1 in stream catalog is produced accurately.
  2. Bar chart # 2 in stream catalog is produced accurately.
  3. Bar chart # 3 in stream catalog is produced accurately.
  4. Bar chart # 4 in stream catalog is produced accurately.
  5. Bar chart # 9 in stream catalog is produced accurately.

I think overall this is more intuitive but it still took me a bit to figure things out. encoding panel labeling modifications will be helpful for this. x is really the study denominator data set and fill is the domain denominator data set and controls grouping. even if you pick "ADSL" as fill data set the counts are number of subjects with event in the ADAE domain in this case.

didn't see how the aggregate function affects output.

Perhaps another way to approach the bar chart is to think about the types of data sets that we have in ADaM. This could be an events domain module and would work for CM and MH. for labs you create a separate module which would work for other BDS analysis data sets. Realize this ties us to ADaM definitions at this point but teal.modules.clinical has that dependency.

Hope this helps.

Provenance: 

Creator: npaszty
cicdguy commented 3 years ago

The aggregation table is not yet implemented, I will do this, so point 5 will be reality once implemented. Were you able to reproduce examples 6 and 7 (numbers according to tlg-catalog)?

I realized that example 5 is also possible (important components highlighted in red):

user/1311/files/5a957d80-6845-11ea-94b7-ea63eb8431bc). In the meantime, I implemented the code necessary for the table in example 8, so it is also possible by now. Except for ADSL, all other datasets are event datasets in the sense that zero or more rows can correspond to one patient. Could you please provide some namings in the interface that are more intuitive to you? Provenance: ``` Creator: mordigm ```
cicdguy commented 3 years ago

@mordigm

Is there a way to remove the NA level from the plot? It doesn't appear to be a level in the filter panel.

user/1100/files/61dc9880-69b7-11ea-9e52-8f32919276c9) For the group labels would use "Primary Grouping" and "Secondary Grouping". For "Statistics" section would use "% Based on Primary Grouping Dataset N" and "% Based on Primary Grouping Column N". Adjust "Notes" section and validation text to reflect updated labeling Y axis note is not super clear. If no Column is selected then the number of patients from the Primary Grouping Dataset is counted? Summary Statistics note is not super clear. I'm still concerned about the intuitiveness of the interface. I think we should run another test with the SMEs to produce the 9 STREAM charts. Maybe best we discuss the feedback via hangouts **Verifications** 1. Bar chart # 5 in stream catalog is produced accurately. 2. Bar chart # 6 in stream catalog is produced accurately. 3. Bar chart # 7 in stream catalog is produced accurately. 4. Bar chart # 8 in stream is produced but when adding the statistics table (mean and median I get the following validation note. "Some patients have non-unique value for column AVAL. Please refine population or unselect aggregation." If I select one of the biomarker groupings then I get the table but that's not the right calculation. we did talk about this a couple of days ago but not sure now. Post comment edit: yes, adding AVISIT filter to one timepoint will produce the chart. let's chat more about that. Provenance: ``` Creator: npaszty ```
cicdguy commented 3 years ago

@npaszty I pushed.

Provenance: 

Creator: mordigm
cicdguy commented 3 years ago

@npaszty Next steps:

Provenance: 

Creator: mordigm
cicdguy commented 3 years ago

@mordigm not sure what happened but I lost all the changes I made to my sub branch. gotta start over. I have bandwidth tomorrow. apologies

Provenance: 

Creator: npaszty
cicdguy commented 3 years ago

@mordigm

I think what happened to my work was that I removed the container in which it was done. lesson learned. Anyway updates pushed to my branch. will need to update the notes under the bar chart but will do that after we discuss feedback.

I think I've made some headway towards a more intuitive interface. I've rearranged some things and broken some things but the I'd like to discuss the concept with you on the approach and then fix the aggregate UI object. I think that will bring us to the finish line and we'll include this in the UAT which is our next sprint. Here are the essentials.

  1. "Primary Grouping" group stays as is
  2. Create a "Primary Count Dataset" group but it only needs to be a select list of data sets. "ADAE", "ADLB". Essentially lifting the Dataset select from the "y-axis" panel. This drives the data specific counts of subjects.
  3. "y-axis" panel no longer needs the Dataset select. It only needs the dynamic list of variables based on which dataset is selected in the "Primary Count Dataset". Now if AETOXGR is selected here then the conditionalPanel would author the lowest:highest radio button group. This moves the reactivity to this group instead of triggered by the "Secondary Grouping" group. If ADLB is selected as the "Primary Count Dataset" then this select would be populated with a list of variables from ADLB like AVAL. If that was the case then the "Aggregate" group would display and the right hand data filters can be used to select "ALT", "HIGH" & "BASELINE" for example to display the BSD type visualizations. Will need to come up with a better label for the "y-axis" object but let's focus on functionality first.
  4. "Secondary Grouping" group stays as is except it no longer drives the reactivity in the y-axis group for conditionalPanel.
  5. The "Aggregation" group no longer displays but that's my doing somehow. Not important for now and would like to discuss items 1 to 4 with you tomorrow.

One enhancement I'd like is to be able to control the y-axis range. If multiple bar charts are generated then consistent y-axis range is important so that bar charts can be placed side-by-side on a slide and the y-axis ranges need to be the same for this. This slider can be added to the "Plot Settings" panel.

I know this is a lot but I think one of the challenges towards creating an intuitive interface is the essential duplication of the "Dataset" and "Column(s)" objects. I don't think we need this and perhaps it means we can't use the data_extract_spec function.

Provenance: 

Creator: npaszty
cicdguy commented 3 years ago

@npaszty I pushed again. You can think about how to update the Notes: section in the Shiny app, I haven't added your changes to this section. The y-range can now be set dynamically or manually so you can compare different graphs with the same y ranges.

Provenance: 

Creator: mordigm
cicdguy commented 3 years ago

@mordigm thanks Max. I think we're getting there on intuition but have some items missing to be able to create all 9 bar charts.

  1. Primary Grouping: works as expected
  2. Primary Count Dataset: the primary count data set drives what the column labels are. so without having anything selected in the Column(s) it should report out the number of unique subjects in the ADAE dataset. currently it reports out the number of subjects in ADSL. When the app launches and default is ADAE then no Column(s) value should need to be selected to have the N annotation above the treatment bars.
  3. the secondary grouping should work but highest/lowest is only relevant to the AETOXGR. user should right hand filter out for AESER for example to produce bar chart 1-5, 8 & 9. Can't produce bar chart 6 or 7 anymore. secondary grouping needs to also be associated with ADSL so we need anothe checkbox in the "Primary Grouping" group as well as what you have in the "Primary Count Dataset" group.
  4. what happened to the other slider? that allowed the fine tuning of the bar labels.

the two items we need to bring back are the ability to create secondary grouping for ADSL for bar chart 6 & 7 and the slider that used to be there. almost there!

Provenance: 

Creator: npaszty
cicdguy commented 3 years ago

@npaszty

  1. I would then recommend removing the checkbox again from "Primary Count Dataset" and adding back the "Secondary grouping" below Primary Count Dataset. Adding a checkbox is not possible for primary grouping because the column needs to be selected for secondary grouping (e.g. SEX, COUNTRY, or similar). Regarding highest/lowest, these can be adapted to work for more use cases. For ADLB, this selection does not appear.

  2. Which slider are you talking about? I removed the slider to adjust the y scale by a percentage and instead added a slider to specify the y range.

  3. When no column is selected in the count dataset, you still need to select a column from this dataset. The column you select should not contain NA. A patient is counted whenever its value in the column is not NA. Internally, since the datasets are merged, this is used to identify which patients are only in ADSL and not ADAE. I would leave it as it is. Anyways, the plot title changes to warn about the unfiltered dataset.

Provenance: 

Creator: mordigm
cicdguy commented 3 years ago

@mordigm General

STREAM Barcharts

  1. created accurately
  2. created accurately
  3. created accurately
  4. created accurately
  5. created accurately
  6. created accurately
  7. created accurately
  8. created accurately. in STREAM the mean is first and median is second in the summary table
  9. can create plot layout but need to address the low/high filtering so that plot can be accurately created.

almost there!

Provenance: 

Creator: npaszty
cicdguy commented 3 years ago

@npaszty

  1. To make it clearer, I will consider putting the filtering of low/high to the right hand side, I will discuss this with Adrian now.

Provenance: 

Creator: mordigm
cicdguy commented 3 years ago

@mordigm From your chat: should I now make it ready for the release? I have already started cleaning up a bit, but still need to do some work if we want to include it.

Let's plan on UATing with the rest of the items for the release sprint. I'm still not clear on how we will handle the low/high bit.

Provenance: 

Creator: npaszty
cicdguy commented 3 years ago

@mordigm can you tell me where we're at with the bar chart module low/high bit?

Provenance: 

Creator: npaszty
cicdguy commented 3 years ago

@npaszty I cleaned the code, devtools check passes, but yet needs to be merged into pre-release. low/high is not resolved yet, I talked to Adrian about it, but we couldn't get to an agreement. The change is non-negligible and we may only include it in the next release. I have already built a small prototype. For me, it makes sense to put this highest/lowest to the right panel, it does not belong to the left encoding panel. Here is a screenshot:

user/1311/files/32cd8e80-78b4-11ea-9e7c-b21ccb77bf0f"> To make it happen, please also push Adrian to include it in the next sprint. PR #297 Provenance: ``` Creator: mordigm ```
cicdguy commented 3 years ago

I created two PRs, towards pre-release and devel. The PR to pre-release has merge conflicts, to devel works.

Provenance: 

Creator: mordigm
cicdguy commented 3 years ago

@mordigm okay thanks. sorry didn't get to this today due to UAT of other stuff. It sounds like this is going to move out of this sprint. Adrian and I have a meeting tomorrow to go over the release print project board.

Provenance: 

Creator: npaszty