diff --git a/spec_tests/jsonTests.spec.js b/spec_tests/jsonTests.spec.js index 2a46b681..7da7fe18 100644 --- a/spec_tests/jsonTests.spec.js +++ b/spec_tests/jsonTests.spec.js @@ -21,9 +21,7 @@ const runAll = true const runMap = new Map([['TAG_EXPRESSION_REPEATED', ['tags-duplicated-across-multiple-rows']]]) //const runOnly = new Set(["eventsPass"]) const runOnly = new Set() -const skippedErrors = { - SIDECAR_KEY_MISSING: 'Warning not being checked', -} +const skippedErrors = {} const readFileSync = fs.readFileSync const test_file_name = 'javascriptTests.json' // const test_file_name = 'temp6.json' diff --git a/src/bids/types/json.js b/src/bids/types/json.js index a4dcf68c..d384419b 100644 --- a/src/bids/types/json.js +++ b/src/bids/types/json.js @@ -168,8 +168,8 @@ export class BidsSidecar extends BidsJsonFile { if (sidecarValue.isValueKey) { this.hedValueStrings.push(sidecarValue.valueString) this.hedData.set(key, sidecarValue.valueString) - } else { - this.hedCategoricalStrings.push(...Object.values(sidecarValue.categoryMap)) + } else if (sidecarValue.categoryMap) { + this.hedCategoricalStrings.push(...sidecarValue.categoryMap.values()) this.hedData.set(key, sidecarValue.categoryMap) } } @@ -283,7 +283,7 @@ export class BidsSidecarKey { /** * The unparsed category mapping. - * @type {Object} + * @type {Map} */ categoryMap @@ -333,7 +333,7 @@ export class BidsSidecarKey { } else if (!isPlainObject(data)) { IssueError.generateAndThrow('illegalSidecarHedType', { key: key, file: sidecar.file.relativePath }) } else { - this.categoryMap = data + this.categoryMap = new Map(Object.entries(data)) } } @@ -378,7 +378,7 @@ export class BidsSidecarKey { this.parsedCategoryMap = new Map() const errors = [] const warnings = [] - for (const [value, string] of Object.entries(this.categoryMap)) { + for (const [value, string] of this.categoryMap) { const trimmedValue = value.trim() if (ILLEGAL_SIDECAR_KEYS.has(trimmedValue.toLowerCase())) { IssueError.generateAndThrow('illegalSidecarHedCategoricalValue') diff --git a/src/bids/validator/tsvValidator.js b/src/bids/validator/tsvValidator.js index 4a32380a..39006b32 100644 --- a/src/bids/validator/tsvValidator.js +++ b/src/bids/validator/tsvValidator.js @@ -59,8 +59,42 @@ export class BidsHedTsvValidator extends BidsValidator { return } this.validateDataset(bidsEvents) - if (this.errors.length === 0) { - //this.issues.push(...this.check_missing_keys()) + if (this.errors.length === 0 && this.bidsFile.mergedSidecar?.hasHedData) { + this._checkMissingHedWarning() + this._checkMissingValueWarnings() + } + } + + _checkMissingHedWarning() { + // Check for HED column used as splice but no HED column + if (this.bidsFile.mergedSidecar.columnSpliceReferences.has('HED') && !this.bidsFile.parsedTsv.has('HED')) { + this.warnings.push(BidsHedIssue.fromHedIssue(generateIssue('hedUsedAsSpliceButNoTsvHed'), this.bidsFile.file)) + } + } + + /** + * Check for categorical column value in tsv but not in sidecar. + * @private + */ + _checkMissingValueWarnings() { + for (const columnName of this.bidsFile.parsedTsv.keys()) { + const sidecarColumn = this.bidsFile.mergedSidecar?.sidecarKeys.get(columnName) + if (!sidecarColumn || sidecarColumn.isValueKey) { + continue + } + const toRemove = new Set(['', 'n/a', null, undefined]) + const tsvColumnValues = new Set(this.bidsFile.parsedTsv.get(columnName)) + const cleanedValues = new Set([...tsvColumnValues].filter((value) => !toRemove.has(value))) + const missingValues = [...cleanedValues].filter((value) => !sidecarColumn.categoryMap.has(value)) + if (missingValues.length > 0) { + const values = '[' + missingValues.join(', ') + ']' + this.warnings.push( + BidsHedIssue.fromHedIssue( + generateIssue('sidecarKeyMissing', { column: columnName, values: values }), + this.bidsFile.file, + ), + ) + } } } @@ -423,7 +457,7 @@ export class BidsHedTsvParser { const columnString = columnValues.hedString.replace('#', rowColumnValue) columnMap.set(columnName, columnString) } else if (columnValues instanceof Map) { - columnMap.set(columnName, columnValues.get(rowColumnValue).hedString) + columnMap.set(columnName, columnValues.get(rowColumnValue)?.hedString) } } diff --git a/src/issues/data.js b/src/issues/data.js index 0f1ca98a..3cc11abd 100644 --- a/src/issues/data.js +++ b/src/issues/data.js @@ -451,7 +451,12 @@ export default { sidecarKeyMissing: { hedCode: 'SIDECAR_KEY_MISSING', level: 'warning', - message: stringTemplate`Key "${'key'}" was referenced in column "${'column'}" of file "${'file'}", but it was not found in any associated sidecar.`, + message: stringTemplate`Values "${'values'}" appear in column "${'column'}" of file "${'file'}", but were not defined in any associated sidecar.`, + }, + hedUsedAsSpliceButNoTsvHed: { + hedCode: 'SIDECAR_KEY_MISSING', + level: 'warning', + message: stringTemplate`Key "{HED}" was referenced in sidecar for file "${'file'}", but this file does not have a HED column.`, }, illegalSidecarHedType: { hedCode: 'SIDECAR_INVALID', diff --git a/src/schema/entries.js b/src/schema/entries.js index e4cf539d..3750e4e8 100644 --- a/src/schema/entries.js +++ b/src/schema/entries.js @@ -849,14 +849,6 @@ export class SchemaTag extends SchemaEntryWithAttributes { return this._valueClasses.slice() } - /** - * Whether this tag has any value classes. - * @returns {boolean} - */ - get hasValueClasses() { - return this._valueClasses.length !== 0 - } - /** * This tag's value-taking child tag. * @returns {SchemaValueTag} diff --git a/tests/bidsTests.spec.js b/tests/bidsTests.spec.js index a0664752..9bc0943c 100644 --- a/tests/bidsTests.spec.js +++ b/tests/bidsTests.spec.js @@ -14,7 +14,7 @@ import { DefinitionManager } from '../src/parser/definitionManager' //const skipMap = new Map([['definition-tests', ['invalid-missing-definition-for-def', 'invalid-nested-definition']]]) const skipMap = new Map() const runAll = true -const runMap = new Map([['curly-brace-tests', ['valid-HED-column-splice-with-n/a']]]) +const runMap = new Map([['curly-brace-tests', ['valid-curly-brace-in-bidsFile-with-value-splice']]]) describe('BIDS validation', () => { const schemaMap = new Map([['8.3.0', undefined]]) diff --git a/tests/testData/bidsTests.data.js b/tests/testData/bidsTests.data.js index 4e566cd7..9ff556b4 100644 --- a/tests/testData/bidsTests.data.js +++ b/tests/testData/bidsTests.data.js @@ -720,7 +720,12 @@ export const bidsTestData = [ eventsString: 'onset\tduration\tevent_code\n' + '19\t6\tball\n', sidecarErrors: [], tsvErrors: [], - comboErrors: [], + comboErrors: [ + BidsHedIssue.fromHedIssue(generateIssue('hedUsedAsSpliceButNoTsvHed', {}), { + path: 'valid-HED-curly-brace-but-tsv-has-no-HED-column.tsv', + relativePath: 'valid-HED-curly-brace-but-tsv-has-no-HED-column.tsv', + }), + ], }, { testname: 'invalid-curly-brace-column-slice-has-no hed', @@ -2111,4 +2116,78 @@ export const bidsTestData = [ }, ], }, + { + name: 'sidecar-key-missing-tests', + description: 'Dataset level tests of warnings for missing sidecar keys.', + tests: [ + { + testname: 'tsv-has-categorical-value-missing-from-sidecar', + explanation: '(Warning) A categorical value in the tsv is missing from the sidecar.', + schemaVersion: '8.3.0', + definitions: ['(Definition/Acc/#, (Acceleration/# m-per-s^2, Red))', '(Definition/MyColor, (Label/Pie))'], + sidecar: { + event_code: { + HED: { + face: 'Acceleration/5', + square: 'Black, Blue', + }, + }, + }, + eventsString: + 'onset\tduration\tevent_code\tHED\n4.5\t0\tface\tBlue\n5.0\t0\tball\tGreen, Def/MyColor\n5.5\t0\tbat\tRed\n', + sidecarErrors: [], + tsvErrors: [], + comboErrors: [ + BidsHedIssue.fromHedIssue( + generateIssue('sidecarKeyMissing', { column: 'event_code', values: '[ball, bat]' }), + { + path: 'tsv-has-categorical-value-missing-from-sidecar.tsv', + relativePath: 'tsv-has-categorical-value-missing-from-sidecar.tsv', + }, + ), + ], + }, + { + testname: 'hed-used-as-splice-but-no-tsv-hed', + explanation: '(Warning) {HED} appears in sidecar but no HED column in tsv,', + schemaVersion: '8.3.0', + definitions: ['(Definition/Acc/#, (Acceleration/# m-per-s^2, Red))', '(Definition/MyColor, (Label/Pie))'], + sidecar: { + event_code: { + HED: { + face: '{HED}', + ball: 'Red', + }, + }, + }, + eventsString: 'onset\tduration\tevent_code\n4.5\t0\tface\tBlue\n5.0\t0\tball\n', + sidecarErrors: [], + tsvErrors: [], + comboErrors: [ + BidsHedIssue.fromHedIssue(generateIssue('hedUsedAsSpliceButNoTsvHed', {}), { + path: 'hed-used-as-splice-but-no-tsv-hed.tsv', + relativePath: 'hed-used-as-splice-but-no-tsv-hed.tsv', + }), + ], + }, + { + testname: 'hed-used-as-splice-and-tsv-has-hed', + explanation: '{HED} appears in sidecar and the tsv has a HED column,', + schemaVersion: '8.3.0', + definitions: ['(Definition/Acc/#, (Acceleration/# m-per-s^2, Red))', '(Definition/MyColor, (Label/Pie))'], + sidecar: { + event_code: { + HED: { + face: '{HED}', + ball: 'Red', + }, + }, + }, + eventsString: 'onset\tduration\tevent_code\tHED\n4.5\t0\tface\tBlue\n5.0\t0\tball\tGreen, Def/MyColor\n', + sidecarErrors: [], + tsvErrors: [], + comboErrors: [], + }, + ], + }, ]