bids-standard / bids-validator

Validator for the Brain Imaging Data Structure
https://bids-standard.github.io/bids-validator/
MIT License
180 stars 109 forks source link

Differences in legacy and schema RepetitionTime check. #2091

Closed rwblair closed 3 weeks ago

rwblair commented 3 weeks ago

https://neurostars.org/t/bids-naming-question-code-1-not-included/30005/9

Legacy cast to number and limits precision:

        const niftiTR = Number(repetitionTime).toFixed(3)
        const jsonTR = Number(mergedDictionary.RepetitionTime).toFixed(3)
        if (niftiTR !== jsonTR) {

Schema on the other hand has:

    - sidecar.RepetitionTime == nifti_header.pixdim[4]
effigies commented 3 weeks ago

We can either add rounding to the language, or we can hack it into loading the sidecar and NIfTI headers. The latter is faster. The former might be more useful to ensure consistency across implementations?

mateuszpawlik commented 3 weeks ago

In Deno validator I get "[ERROR] REPETITION_TIME_MISMATCH Repetition time did not match between the scan's header and the associated JSON metadata file."

For example:

effigies commented 3 weeks ago

Okay, thinking about a rounding function, we could either explicitly create a round(x: T, digits: number) -> T, or we could extend allequal(x: T[], y: T[], tol?: number) -> boolean.

Case round() allequal()
number round(x, 3) == round(y, 3) allequal([x], [y], 1e-3)
number[] allequal(round(x, 3), round(y, 3)) allequal(x, y, 1e-3)

Any preference? I like keeping the language small, but this might be over-complicating the allequal() function.

mateuszpawlik commented 3 weeks ago

I don't know much about MRI data myself. I'm wandering why the Repetition Time values would be different in NIFTI header and JSON file and still correct. Shouldn't they be exactly the same? From the Neurostart post, I understand that they should be the same. In such a case, I don't understand the need for rounding.

We generate the JSONs with dcm2niix. I assume the values are equal, and they are when I check. That would mean that the validator does something to fail the equality test of equal numbers :thinking:

effigies commented 3 weeks ago

The JSON value is a decimal string that will generally be converted to a float64 on parsing. The NIfTI-1 header uses float32 for its pixdim array. Since not all decimals can be exactly represented as floating points, you have the opportunity for error:

>>> import numpy as np
>>> import json
>>> RepetitionTime = json.loads('1.299')
>>> pixdim4 = np.float32(RepetitionTime)
>>> pixdim4 == RepetitionTime
False

Equality checks for floating points require tolerances, one way or another, or you'll eventually hit a case like this.

mateuszpawlik commented 3 weeks ago

Interesting. Thanks for explaining. I didn't expect that. But then the problem is already in converting DICOMs to NIFTIs and writing the JSON files, because there are two representations. I'm sorry, I didn't want to trigger any unnecessary discussion.

rwblair commented 3 weeks ago

Without adding anything to the schema language would something like the following work? -0.01 < (sidecar.RepetitionTime - nifti_header.pixdim[4]) < 0.01

effigies commented 3 weeks ago

Since checks are implicitly ANDed together, we could do:


checks:
  - sidecar.RepetitionTime - nifti_header.pixdim[4] < 0.01
  - sidecar.RepetitionTime - nifti_header.pixdim[4] > -0.01
effigies commented 3 weeks ago

@rwblair Adopting this approach in https://github.com/bids-standard/bids-specification/pull/1893: https://github.com/bids-standard/bids-specification/commit/cf48a47e2f939b04cef90513c54e831594999700

mateuszpawlik commented 2 weeks ago

FYI I wanted to try it out and used the command as in https://github.com/bids-standard/bids-validator/issues/2081#issuecomment-2296830932 and got this error:

docker run -ti --rm -v $PWD:/data:ro denoland/deno deno run --allow-read --allow-env --reload -A https://github.com/bids-standard/bids-validator/raw/deno-build/bids-validator.js /data
error: Uncaught (in promise) Error: Dynamic require of "events" is not supported
  throw Error('Dynamic require of "' + x + '" is not supported');
        ^
    at https://raw.githubusercontent.com/bids-standard/bids-validator/deno-build/bids-validator.js:12:9
    at Object.<anonymous> (https://raw.githubusercontent.com/bids-standard/bids-validator/deno-build/bids-validator.js:11:12)
    at ../../../../.cache/deno/deno_esbuild/xml2js@0.6.2/node_modules/xml2js/lib/parser.js (https://raw.githubusercontent.com/bids-standard/bids-validator/deno-build/bids-validator.js:395:4)
    at __require3 (https://raw.githubusercontent.com/bids-standard/bids-validator/deno-build/bids-validator.js:15:50)
    at Object.<anonymous> (https://raw.githubusercontent.com/bids-standard/bids-validator/deno-build/bids-validator.js:12:12)
    at ../../../../.cache/deno/deno_esbuild/xml2js@0.6.2/node_modules/xml2js/lib/xml2js.js (https://raw.githubusercontent.com/bids-standard/bids-validator/deno-build/bids-validator.js:39:4)
    at __require3 (https://raw.githubusercontent.com/bids-standard/bids-validator/deno-build/bids-validator.js:15:50)
    at https://raw.githubusercontent.com/bids-standard/bids-validator/deno-build/bids-validator.js:6:25