Initial missing values implementation #110
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
surchs
reviewed
Apr 28, 2022
There was a problem hiding this comment.
Thanks @jarmoza for working on this!
I think this looks good as a component. I have two comments.
- I think the missing values component doesn't have to / should not guess what a missing value is. In most cases we will let the user tell us (only exception I can see is for numerical data). So it could be just a dumb "state viewer". Some other component tells the state that a bunch of values are "missing" and then this component know what the "relevant" columns currently are and displays for them the "missing values". Then it only has to handle the case of "nope, this is actually not a missing value after all". That way we can keep the "guess missing values" logic with the corresponding categories (e.g. age annotation) and also keep the list of "guessers" short. Another reason for this is that guessing seems pretty error prone, and I think we should avoid that complexity until we know that it is necessary.
- relatedly, there currently doesn't seem to be a way to designate a value "missing" (i.e. you start with a list based on the guesses and can remove from there). Maybe we could just have the missing-value-component listen to the missing-value-list in the state and teach the other components to emit "this is now a missing value" events to the state.
What do you think?
| console.log(`WARNING: Could not find '${p_columnName}' in p_state.missingColumnValues`); | ||
| } | ||
|
|
||
| // Value is considered missing if it is not in the store's missing values list |
There was a problem hiding this comment.
shouldn't it read "value is considered missing if it is in the store's missing values list"?
|
|
||
| // Value is considered missing if it is not in the store's missing values list | ||
| // This is determined via determineMissingValues() and later on, the user via this component | ||
| return ( p_state.missingColumnValues[p_columnName].includes(p_value) ); |
There was a problem hiding this comment.
does this line fail if the p_columnName is not in p_state.missingColumnValues?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm setting this initial version of the missing values component up as a PR. It's currently functional, but a code and functionality review is warranted.