-
Notifications
You must be signed in to change notification settings - Fork 6
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
getOptions getter for Phase 2 store refactor #353
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @jarmoza. All looks good, but I think we'll need to change where the categorical options are coming from, "Levels"
or even the data dictionary in general don't make sense. I left a more detailed comment inline.
Ah, missed this. The |
Co-authored-by: Sebastian Urchs <surchs@users.noreply.github.com>
Co-authored-by: Sebastian Urchs <surchs@users.noreply.github.com>
@surchs Will add a comment about possible different place for the `transformationHeuristics store field. |
@surchs Take a look at my recent commits. I believe they address the issues mentioned above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jarmoza, I agree. This looks good to go. I left two comments for my own clarity, but go ahead and merge when you've seen them.
This closes #276.
This PR implements two new store getters
getCategoricalOptions
andgetTransformOptions
for the phase 2 store refactor, tests for them, and necessary refactors to components/tests that use these getters.These getters handle retrieving options for categorical columns (via the
Levels
key in a column indataDictionary.userProvided
) and continuous values categories (soon to be for columns) from a new store fieldtransformationHeuristics
.Unit tests have been implemented for current option retrieval scenarios in
store-getters-getCategoricalOptions.cy.js
andstore-getters-getTransformOptions.cy.js
.Both the
annot-continuous-values
andannot-categorical
components have been lightly refactored to make use of these new getters. (Additionally, some formatting changes have been made in these component/test files to accord with code formatting in the repo.)At least one open question for this PR is a point of discussion as to whether or not this new store field
transformationHeuristics
is a reasonable spot and configuration for storing transform heuristic options.