Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancement: Column Mapping Feature #140

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

js3110
Copy link
Collaborator

@js3110 js3110 commented Dec 3, 2024

Issue

Closes #112

Description

We would like to offer the user better possibilities to define the column names that correspond with each column used by the app in the data. This will increase substantially the data flexbility and will also provide the user a better understanding of the App functioning

Definition of Done

The requirements for the feature to be complete:

  • Provide select/selectize inputs for the user to define each of the important columns. Preferably if the layout is nice and with easy searchable options. One proposal is divided in these groups:

  • Group identifiers. Subject ID, Dose Number, Additional grouping

  • Concentration variables. Analyte, Analyte value

  • Dose variables. Drug, Dose, Administration route

  • Time variables. Time from 1st dose, Time from last dose, Dose duration

  • Unit variables. Concentration unit, Time unit

  • Hovering displays information about the column requirements or details. For example for administration route (administration preferably stating either "intravascular" or "extravascular")

  • Provide inputs for the user to define the columns for unit measures (e.g, AVALU)

  • Formulas for PKNCAdata and PKNCAdose need to include grouping_variables

How to test

Data Section -> Column mapping
Try using dataset with renamed columns to see that it still works.

Contributor checklist

  • Code passes lintr checks
  • Code passes all unit tests
  • New logic covered by unit tests
  • New logic is documented

@js3110 js3110 linked an issue Dec 3, 2024 that may be closed by this pull request
4 tasks
@js3110 js3110 marked this pull request as ready for review December 4, 2024 08:36
@js3110 js3110 requested a review from Gero1999 December 4, 2024 08:37
@js3110
Copy link
Collaborator Author

js3110 commented Dec 4, 2024

@Gero1999 I switched to bslib tooltips because the shinyBS ones weren't working. Basically the same thing though. I've left the tooltip messages pretty vague for now, only the grouping and adosedur ones have more info. Lmk if you want any of them to change. I figure once we add labels from #100 then some of the tooltips won't be needed or we can adjust them as needed.

Also not sure about grouping_variables as part of pkconc and pkdose? can also be adjusted later, after you've merged #138

Copy link
Collaborator

@Gero1999 Gero1999 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice @js3110! I agree, once #138 is merged can be clever to considered, is just about adding the grouping_varaibles to both formulas.

Some feedback regarding optional details that could be added:

  • I would prefer that the input calls are static and created in the UI (i.e, selectInput(...))). Otherwise it is very likely to produce issues, I guess that this could be the reason of why shinyBS was not working for you.
  • For our users label ID would be preferred to be explanatory names rather than CDISC variables.
  • Hovering can add information about the column expected format, i.e, class expected such as numeric or categorical.
  • When the user press Submit Mapping I would expect something to happen. Either a message notification like "Data mapped" or a Tab displacement
  • grouping variables could be better called something like other grouping variables so the user do not get the wrong perception and thinks the rest are not used and puts them two times. it could also be good to update the choices to elude them to put the same column as in ANALYTE, DOSNO...

@js3110 js3110 requested a review from Gero1999 December 5, 2024 09:50
@js3110
Copy link
Collaborator Author

js3110 commented Dec 5, 2024

@Gero1999 have updated. its a lot more bulky now but the ui is static. the label IDs will be added when we do #100 so I have left it how it was. Not entirely sure what else to put in the tooltips but feel free to make suggestions if you think it needs changing. For grouping variables I have added a clearer description so the user knows what it is for, and if the columns are already named correctly the user cannot select the other columns anyway.

@Gero1999
Copy link
Collaborator

Gero1999 commented Dec 6, 2024

Good job! I just detected some small things. Let me know if I can help and you agree with them:

  • Main task incomplete: Add in formulas for PKNCAdata and PKNCAdose need to include grouping_variables
  • App crashes when using the same column name for different inputs! We should either disable this option or prevent the crash wiht a notification!
  • ADOSEDUR should be simply set to 0 when a study is not infusion. I am not sure if the NA is actually currently doing anything (the column is not ignored, simply not renamed).

@js3110
Copy link
Collaborator Author

js3110 commented Dec 9, 2024

Good job! I just detected some small things. Let me know if I can help and you agree with them:

  • Main task incomplete: Add in formulas for PKNCAdata and PKNCAdose need to include grouping_variables
  • App crashes when using the same column name for different inputs! We should either disable this option or prevent the crash wiht a notification!
  • ADOSEDUR should be simply set to 0 when a study is not infusion. I am not sure if the NA is actually currently doing anything (the column is not ignored, simply not renamed).

@Gero1999 Ok it should be ready now, as agreed I have left grouping_variables for nca to you for after #60 has been completed. And for the input labels, again this will be included in #100 .

@Gero1999
Copy link
Collaborator

Gero1999 commented Dec 10, 2024

hey @js3110 as discussed in chat we will accelarate one of the issues into this one directly. Just add an option for the user to apart from choosing column names in the unit inputs also standard concentration variables. Here are some helpful instructions:

  • Take the values directly from PKNCA options if possible, don't hardcode unless you don't see other alternative
  • Make sure to separate them by order and definition in the input. If you use a list object for the select input it will understand how to seggregate the results (dataset columns and measure units)
  • When a measure units is selected, the column is automatically created with only that unit

@js3110
Copy link
Collaborator Author

js3110 commented Dec 11, 2024

@m-kolomanski @Gotfrid Hi, could one of you please check the code on this and review when you get the chance. There is a linter check saying it has too much cyclomatic complexity, so suggestions on how to fix this would be appreciated. Thanks!

@Gotfrid
Copy link
Collaborator

Gotfrid commented Dec 11, 2024

Hi @js3110 , I will be able to do the review earliest tomorrow. Cyclomatic complexity issue means that you have too many if-else branches in a function. In case of Shiny it pretty much always means shiny modules are required to extract pieces of server.

@js3110
Copy link
Collaborator Author

js3110 commented Dec 11, 2024

Hi @js3110 , I will be able to do the review earliest tomorrow. Cyclomatic complexity issue means that you have too many if-else branches in a function. In case of Shiny it pretty much always means shiny modules are required to extract pieces of server.

ok thanks! I will created a nested module today so its ready by tomorrow.

Copy link
Collaborator

@Gero1999 Gero1999 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancement: Customized column definition as inputs
3 participants