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

Add type property for column mappings #563

Closed
jwildfire opened this issue May 27, 2021 · 2 comments
Closed

Add type property for column mappings #563

jwildfire opened this issue May 27, 2021 · 2 comments
Labels
wontfix This will not be worked on

Comments

@jwildfire
Copy link
Contributor

Summary

Capture the type of variable expected for each column mapping in meta, and then somehow enforce those types in the app. For example, id_col should always be character, and labs$measure_col should always be numeric.

Details

Version 1 of the app had a complicated system for data checks that were almost definitely overkill and has been removed in version 2. This can definitely lead to problems and confusion when loading dirty data (and broken charts) though (as @JimBuchanan and @xni7 noted in recent conversations). Capturing the expected type for columns is easy enough, but deciding what to do with that information in the app is more complex.

A few options:

  1. We could only show columns with the required type in the mapping section dropdowns (e.g. the id_col dropdown would only show character (and factor??) variables)
  2. Show all variables in mapping dropdowns, but then coerce variables to the required type before drawing charts. (e.g. if a numeric column is selected for id_col, but it's coerced to character)
  3. Show a warning in the mapping section if a variable isn't the correct type.
  4. Leave the app as is, but implement checks in the initialization process.

Certainly open to other suggestion, but I'd lean towards option 3 above since it would be pretty easy to implement.

Two final notes:

  • In an ideal world, the charts would deal with this kind of check. If a non-numeric value is provided for a numeric column, the chart should throw an error. This happens sometimes in our htmlwidgets, but it's generally in the error logs and not visible to the user.
  • There actually is a type variable in meta that specifies whether the mapping is field or column level. I think we'd want to rename the existing variable if we implement this.
@jwildfire
Copy link
Contributor Author

This is another one where I'm on the fence about implementation for v2.0. Input welcome!

@jwildfire jwildfire added the wontfix This will not be worked on label Aug 25, 2021
@jwildfire
Copy link
Contributor Author

The more I think about this, the more I think chart-level requirements and validation is the way to go as described here.

Putting a type on the domain-level metadata is somewhat limiting. For example, it's not at all crazy to think that some charts would support character lab results ("positive", "negative", "below LOD", etc) while others (like hep-explorer) would only support numeric values.

Going to mark this as wontfix and close, though we can certainly revisit later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

1 participant