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

[FEAT] add discrete value annotation #87

Merged
merged 8 commits into from
Mar 25, 2022

Conversation

surchs
Copy link
Contributor

@surchs surchs commented Mar 24, 2022

This PR adds a drop-down based functionality for mapping discrete values to a set of available choices.
For now we use this only for the sex category

- vue-bootstrap does not have a good option for drop-down selection with string autocompletion so we're using https://vue-select.org/ here instead
- linked to the category-sex.vue sub-page
- passing (for now) hardcoded data
- this allows us to map values like those in the sex category to a set of predefined choices.
- connected to the annotation page
- added a button to trigger the updating of the datatable
@surchs surchs requested a review from jarmoza March 24, 2022 22:11
@surchs
Copy link
Contributor Author

surchs commented Mar 24, 2022

ok @jarmoza, I think this works now as we would like it to. Ready for your review!

@jarmoza
Copy link
Contributor

jarmoza commented Mar 25, 2022

@surchs Just tested this. I added categorized as 'sex' the actual 'sex' column and two other columns. Proceeding to annotation, I mark the 'M' as 'male' and the 'F' as 'female' and the blank value as 'missing value'. Confirm and Upload remains disabled as expected – because the other columns have not been annotated with accepted 'sex' values.

However, when I removed the miscategorized columns the button remains disabled. This means that column removal must be able to indicate that the Confirm and Upload button should be enabled/disabled.

One good point on that last action is that those miscategorized columns are actually unlinked from the 'sex' category on the categorization page when I navigate back to it.

@jarmoza
Copy link
Contributor

jarmoza commented Mar 25, 2022

@surchs Let me know if you have any thoughts on my recent comment. The easiest way would be to utilize the download page accessible flag in pageData similar to the previous pages.

What could/should be happening is that after a function that could change the page state - i.e. clear to proceed to the download page - a call should be made to change the page's state and in turn the actions that should follow that particular state change.

@surchs
Copy link
Contributor Author

surchs commented Mar 25, 2022

@jarmoza: what data did you test this with? One of our example files?
OK, I can see that the button never gets enabled. Checking now.

@surchs
Copy link
Contributor Author

surchs commented Mar 25, 2022

@jarmoza: I can confirm that this does not currently work:

  1. Label two columns as "about" sex
  2. Go to the annotation page and select the "sex" tab
  3. using the annotated-columns component, delete one of the two columns
  4. label all the unique elements of the remaining column
  5. expect the button to become enabled -> it does not

The problem was that the method to enable / disable the button did not react to the number of columns changing and was still checking whether the unique values of the deleted column would be annotated (which they of course could never be), thus being forever stuck in a "not all annotated" state.

One fix for this is to just recreate the valueMapper that is being checked to determine the annotation status with a watcher whenever the columns change. This works, in the sense that now we determine the annotation status only based on the currently selected columns.

However, we now have another problem: because we are using the v-select dropdown component inside scoped slots, and because v-model does not work inside scoped slots (see this stackoverflow and the linked issue from vue) we are using the @input event directly to react to a dropdown value being selected. The selected value is still modeled with v-model inside the dropdown component, but no longer tied to the state of the app. So when we re-initialize the valueMapper as empty via the watch function by removing a column, an already selected value in the dropdown does not get reset as well. So you could end up with the very confusing situation where all dropdown values have some string selected but the button is disabled because internally the mapping is emtpy.

This does not happen if you navigate away from the page and come back, but that's no good. So in short, I think we need to not throw out the valueMapper and re-initialize as emtpy, but rather pick and choose what should remain and what should be removed.

- before this commit, the value mapper was initialized onMount with all the columns mapped to the activeCategory at that time
- if the relevant columns change later (particulalry when one is deleted), the valueMapper wouldn't update
- this meant that we would never be able to enable the upload button because it would always be waiting for the unique elements of the deleted column to be annotated.

Changes:
- added a watcher for relevantColumns computed prop
- when relevantColumns changes, the valueMapper gets updated
- we detect the columns that were removed and delete the corresponding keys from the ValueMapper
- to delete the keys, we use the Vue.delete method for all this to remain reactive.
Copy link
Contributor

@jarmoza jarmoza left a comment

Choose a reason for hiding this comment

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

Now the functionality is fixed - and this is good enough for the demo. Obviously we are going to want to revisit internal implementation and other issues with this code post-demo.

I'm going to refrain from making detailed comments on the code with that stipulation.

@surchs
Copy link
Contributor Author

surchs commented Mar 25, 2022

OK, thanks @jarmoza. I will merge this now then.

@surchs surchs merged commit 333fb58 into master Mar 25, 2022
@surchs surchs deleted the feat-add-discrete-value-annotation branch March 25, 2022 19:12
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.

2 participants