Implement missing values vocabulary #143

surchs · 2022-05-09T23:42:50Z

This PR adds a UI element to the diagnosis (vocabulary) component to declare values missing and hook these actions up to the global store.

Work done:

there is an interface element next to each unique value that allows me to designate the value as "missing"
when I use this element, the value will be removed (but only the combination of value + column, not any values with the same name from other columns) from the list of values to be annotated
the value will then also show up in the missing values component
removing missing values should not block/break the ability to complete the annotation status (e.g. only non-missing values should count towards annotation status)
conversely, if a missing value is added back (e.g. designated not-missing), its mapping should be empty and the annotation status should reflect that (e.g. go from "complete" to "incomplete")
when I save my annotation, missing values get assigned a "missing value" label

Resolves #121

- clicking the button will now emit the update:missingValue event

- this is only implemented for row-mode - when the user declares a value to be missing in the interface by pressing the "missing value" button, this value will be hidden from the annotation view

- made non-reactive assignments to this.vocabularyMapping reactive - changed the logic for the button-enabling function to computed variable (hence requiring the reactivity) - refactored the deep object merge into a method (will have to be eventually refactored out of here so we can reuse across components)

jarmoza

Aside from the handful of comments on the code itself and one significant interface error, all looks good.

The error has to do with this piece of functionality:

conversely, if a missing value is added back (e.g. designated not-missing), its mapping should be empty and the annotation status should reflect that (e.g. go from "complete" to "incomplete")

There exists a scenario where this breaks. Steps to reproduce:

Mark a value as missing (on the diagnosis tab)
Fill out all other non-missing vocabulary term boxes
Place back the missing value (as not missing)
Type in its box
Observe that the "Save Annotation" button remains greyed out and that vocabulary term values (in some test cases) may be shifted to different boxes depending on the order of the initially removed value. Having all boxes filled out no longer enables the 'Save Annotation' button

I am also getting this warning from lines 553 and 575 in index.js:

Could not find 'sex' in p_state.missingColumnValues. Will treat as not missing.

jarmoza · 2022-05-11T20:58:00Z

components/annot-vocabulary.vue

+            saveButtonEnabled() {
+
+                for ( const [columnName, columnMappings] of Object.entries(this.vocabularyMapping) ) {
+                    console.log("val", columnName, columnMappings)


Remove debug line?

jarmoza · 2022-05-11T20:58:44Z

components/annot-vocabulary.vue

+                        // The first time we find any mapped value that is neither non-empty nor missing, we return
+                        // a status of false. Otherwise, we return true.
+                        if (
+                            // We use negate the first conditional tuple so that lazy evaluation of the two conditionals


It's not typical practice to insert a comment within a conditional statement. I suggest just moving this description above the if statement.

The comment is not referring to the if statement but to the logic inside the condition. Could you maybe give a reason why you think we should not put comments inside if statements?

The short answer to this is precedent - it's just not common practice to do so. The better answer though is that the condition should be rewritten for clarity and in doing so likely not need as much commentary.

I believe this original comment

// The first time we find any mapped value that is neither non-empty nor missing, we return // a status of false. Otherwise, we return true. if ( // We use negate the first conditional tuple so that lazy evaluation of the two conditionals // joined by && will skip the second conditional if uniqueValue is null. This is important // because null doesn't have a .trim() method. ! ( mappedValue !== null && mappedValue.trim().length > 0 ) && ( ! this.isMissingValue(columnName, uniqueValue) ) ) { return false; }

Can also be written this way:

// If there is any mapped value that is empty or unique value that is not missing, we return false if ( (mappedValue !== null && mappedValue.trim().length === 0) || !this.isMissingValue(columnName, uniqueValue) ) { return false; }

The reasoning for this as follows. If we map out how your original condition could succeed, we get the following:

mappedValue !== null && mappedValue.trim().length === 0 (T and F) OR mappedValue === null --> short-circuit out of evaluating second condition (F) AND this.isMissingValue(columnName, uniqueValue) === false

@surchs Can you provide some feedback here as to if I have correctly understood the intent of this condition?

Oh, ok. Sorry, I didn't realize that this was a case of comments inside the if clause. Yeah, agreed. That's a mistake. Thanks for catching it. I read that wrong.

Can you provide some feedback here as to if I have correctly understood the intent of this condition?

Yes, almost. I think
mappedValue.trim().length === 0 should be mappedValue.trim().length > 0 as before because a length of 0 would imply an empty string. But with that addition, I agree that your way of writing it is a lot clearer because we get rid of the double negative!

edit: hold on a sec, I think I confused myself here

Ah, hold on. Now I confused myself (goes to show that the if clause needs cleaning). The idea is that we want to turn on/make available the NEXT button when all unique values have been either mapped or declared missing.

So for a uniqueValue, if
the corresponding mappedValue is NOT set (i.e. is null) OR is empty (i.e. is an empty string with length zero)
AND
the uniqueValue has not been marked as a missingValue
THEN
this tells me that at least one value remains unmapped and the button shouldn't be enabled.

(this is the intent, not the literal code)

The problem is that we can't write it like that because if
mappedValue === null is true, then we'll go on to evaluate mappedValue.trim(appedValue !== null).length > 0 and then crash because a null object doesn't have a trim() method.

Your version does a different thing:

if ( (mappedValue !== null && mappedValue.trim().length === 0) || !this.isMissingValue(columnName, uniqueValue) ) {

for a uniqueValue, if
the corresponding mappedValue IS set (i.e. is NOT null) BUT is empty (i.e. is an empty string with length zero)
OR
the uniqueValue has not been marked as a missingValue
THEN
we consider the annotation not complete and the button to not be enabled. But this is not correct, the OR here is wrong.

Aha, yes, I think this is going to be it, with one minor change (length > 0 -> length === 0):

if ( ( mappedValue === null || (mappedValue !== null && mappedValue.trim().length === 0) ) && !this.isMissingValue(columnName, uniqueValue ) { return false; }

And then maybe add a comment on why we cannot just do

if ( ( mappedValue === null || mappedValue.trim().length === 0 ) && ...

so that we don't accidentally "improve" it later and get surprised when things break.

Very cool, thanks for thinking through this!

It had to get done! Good talking through.

Actually @surchs

if ( ( mappedValue === null || mappedValue.trim().length === 0 ) && ...

should work given that if mappedValue is null it won't check the second condition once mappedValue === null evaluates to true. But of course, mappedValue !== null can be left in for clarity even if it's redundant

Ah, that's right. I didn't think about this because now we have turned the statement from !== to ===. OK, in that case I think it is really clear, we might not even need a comment. Look at that!

Excellent work. Exactly where we want it to be.

jarmoza · 2022-05-11T20:58:59Z

components/annot-vocabulary.vue


            // Initialize the mapping of all unique values as null
            this.initializeMapping();
        },

        methods: {

+            declareMissing(p_row) {
+
+                console.log("please declare", p_row, "missing")


Remove debug?

surchs · 2022-05-12T15:46:31Z

I can reproduce the behaviour, but I think it is a display bug. The internal state is correctly represented, but the text boxes are not. Try this:

enter the raw value as vocabulary text (so it's easier to tell the mapping)
remove the top most entry
notice how the mapped values just got shifted down instead of being removed with the row
Finally, check the internal state and see that the mapping was correctly removed

I think this happens because the text fields are not using dynamic ids and are generally not hooked up to the store. This is a problem we currently have for all of the annotation components. Not sure how to proceed. On the one hand I'd like to handle the visual = state problem in a future PR with #128 . On the other hand it's a bad idea to merge things that are not working as intended.

I think I still lean towards having this go in with the display bug now so we can start building with the internal functionality (that is working correctly as far as I can tell) and address the visual issues in parallel. Wdyt @jarmoza

jarmoza · 2022-05-12T16:01:28Z

My initial reaction is that we (you) should spend a little bit of time trying that ID fix to see if it resolves the issue. I'd have to take a look at the code before I could suggest other ideas. I agree it's bad to merge something broken - but this should also not become a blocker for this week's/upcoming work. The stipulation is that this bug is written up into an issue and not lost.

This line is likely from a previous implementation where `tableItems` was a data field. Since it is now a computed property.

surchs · 2022-05-27T16:48:49Z

Thanks for the commits @jarmoza! I can confirm that this takes care of the wandering label issue for the vocabulary mapper! The weird thing though is that the text fields still reset to empty when I navigate away and back to the annotation tab. I'll take another look to see if I can fix that with your v-model solution!

surchs · 2022-05-30T15:08:10Z

Thanks @jarmoza. looks good to merge to me! Thanks a lot for taking care of the v-model

surchs added 5 commits May 9, 2022 19:28

Add missing value button to vocab component

a96ba47

connect missing-value button to logic

fa330d8

- clicking the button will now emit the update:missingValue event

show only unique values that are not declared missing

c6aba63

- this is only implemented for row-mode - when the user declares a value to be missing in the interface by pressing the "missing value" button, this value will be hidden from the annotation view

enable saving annotations with missing values

d08dc56

surchs requested a review from jarmoza May 9, 2022 23:42

jarmoza reviewed May 11, 2022

View reviewed changes

jarmoza added 4 commits May 20, 2022 15:07

Work on fix for incorrectly updating vocab

1809982

V-model for missing values

3f76099

Merge branch 'master' into implement-missing-values-vocabulary

6853e4d

Deleting table row removal line

843f125

This line is likely from a previous implementation where `tableItems` was a data field. Since it is now a computed property.

Value/input props for discrete choices component

b3440dc

surchs merged commit bea4415 into master May 30, 2022

surchs deleted the implement-missing-values-vocabulary branch May 30, 2022 15:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement missing values vocabulary #143

Implement missing values vocabulary #143

surchs commented May 9, 2022

jarmoza left a comment

jarmoza May 11, 2022

jarmoza May 11, 2022

surchs May 13, 2022

jarmoza May 19, 2022

surchs May 19, 2022 •

edited

surchs May 19, 2022

surchs May 19, 2022 •

edited

jarmoza May 19, 2022

jarmoza May 19, 2022 •

edited

surchs May 19, 2022

jarmoza May 19, 2022

jarmoza May 11, 2022

surchs commented May 12, 2022

jarmoza commented May 12, 2022

surchs commented May 27, 2022

surchs commented May 30, 2022

Implement missing values vocabulary #143

Implement missing values vocabulary #143

Conversation

surchs commented May 9, 2022

jarmoza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surchs May 19, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surchs May 19, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jarmoza May 19, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surchs commented May 12, 2022

jarmoza commented May 12, 2022

surchs commented May 27, 2022

surchs commented May 30, 2022

surchs May 19, 2022 •

edited

surchs May 19, 2022 •

edited

jarmoza May 19, 2022 •

edited