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

Getter for json output for discrete (categorical) value columns #426

Merged
merged 11 commits into from
May 17, 2023

Conversation

jarmoza
Copy link
Contributor

@jarmoza jarmoza commented May 12, 2023

Closes #406
See also #408

  • New getter introduced in store getCategoricalJsonOutput that takes a column name and outputs a json object with annotated values for that column that is compliant with the schema as noted in getter to create JSON representation of discrete column #406
  • New unit test file store-getter-getCategoricalJsonOutput.cy.js with tests for schema compliance and value/value formatting checks for the 'IsAbout' and 'Levels' sections of the json output

@jarmoza jarmoza added data store maint:coverage Test coverage improvements that were not included in feature prioritization labels May 12, 2023
@jarmoza jarmoza changed the base branch from master to dev_components_talk_to_store_directly May 12, 2023 20:11
@rmanaem rmanaem self-requested a review May 12, 2023 20:14
@surchs surchs requested review from surchs and removed request for surchs May 12, 2023 20:21
@surchs
Copy link
Contributor

surchs commented May 12, 2023

ah, didn't realize you had already picked this up @rmanaem.

@rmanaem
Copy link
Contributor

rmanaem commented May 12, 2023

@surchs No worries

Copy link
Contributor

@rmanaem rmanaem left a 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! I requested a minor change in one of the test cases, let me know what you think.

@surchs
Copy link
Contributor

surchs commented May 12, 2023

Just mentioning a couple of things I noticed here:

  • this was missing from the spec, but we have a process for "missing values" and they should be part of the output data dictionary. See https://www.neurobagel.org/documentation/dictionaries/#assessment-tool for example.
  • what is stored under valueMap in the store is not the text-label of a categoricalOption, but the unique identifier. See the relevant component test for an example -> we have the identifier, and we need to lookup the human label (not the other way around)
  • although it is true that currently all variables a column can be "isAbout" live in the neurobagel namespace, not all of them will have a unique identifier that is an exact match with their human readable label. I see @jarmoza has already made an issue to align the label with the identifier: Swap 'Subject ID' for 'Participant ID' #425 However I think this will quickly get hard to keep up. I think a better way is to do what we've done for the (currently hardcoded) categorical options and add an identifier attribute to the pre-defined categories (e.g. here). That will allow us to
    • have the identifier clearly spelled out in a single place - reducing mental math to parse it from the code
    • have a variable mapped to a different identifier than it's human label (e.g. something with a space "Participant ID" could map to http://neurobagel.org/vocab/ParticipantID)
    • change to a different, non neurobagel namespace, ...

@rmanaem rmanaem requested a review from surchs May 16, 2023 19:33
Copy link
Contributor

@surchs surchs left a comment

Choose a reason for hiding this comment

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

Thanks a lot @rmanaem for taking on the PR! I read through the whole thing and left some comments. I think the missingValues implementation is assuming the wrong data structure for missingvalues (it's an array, not an object). Could you fix that and take a look at some of the other comments (:cherries: are just nice-to-have's you can ignore).

cypress/unit/store-getter-getCategoricalJsonOutput.cy.js Outdated Show resolved Hide resolved
cypress/unit/store-getter-getCategoricalJsonOutput.cy.js Outdated Show resolved Hide resolved
cypress/unit/store-getter-getCategoricalJsonOutput.cy.js Outdated Show resolved Hide resolved
cypress/unit/store-getter-getCategoricalJsonOutput.cy.js Outdated Show resolved Hide resolved
cypress/unit/store-getter-getCategoricalJsonOutput.cy.js Outdated Show resolved Hide resolved
store/index.js Outdated Show resolved Hide resolved
store/index.js Show resolved Hide resolved
store/index.js Show resolved Hide resolved
store/index.js Show resolved Hide resolved
store/index.js Outdated Show resolved Hide resolved
@rmanaem rmanaem requested a review from surchs May 17, 2023 18:09
Copy link
Contributor

@surchs surchs left a comment

Choose a reason for hiding this comment

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

Nice, looks good @rmanaem. Good to merge 🧑‍🍳

Thanks @jarmoza and @rmanaem for addressing this!

@rmanaem rmanaem merged commit 796ee3a into dev_components_talk_to_store_directly May 17, 2023
@rmanaem rmanaem deleted the jarmoza-406-clean branch May 17, 2023 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data store maint:coverage Test coverage improvements that were not included in feature prioritization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

getter to create JSON representation of discrete column
4 participants