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

Differences in legacy and schema RepetitionTime check. #2091

Closed
rwblair opened this issue Aug 16, 2024 · 10 comments
Closed

Differences in legacy and schema RepetitionTime check. #2091

rwblair opened this issue Aug 16, 2024 · 10 comments
Labels

Comments

@rwblair
Copy link
Member

rwblair commented Aug 16, 2024

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
Copy link
Collaborator

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
Copy link

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:

  • in JSON file: "RepetitionTime": 1.299
  • in NIFTI file's header (printed with nibabel): pixdim : [-1. 2.4615386 2.4615386 2.5 1.299 1. 1. 1.]

@effigies
Copy link
Collaborator

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
Copy link

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 🤔

@effigies
Copy link
Collaborator

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
Copy link

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
Copy link
Member Author

rwblair commented Aug 20, 2024

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

@effigies
Copy link
Collaborator

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
Copy link
Collaborator

@mateuszpawlik
Copy link

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/[email protected]/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/[email protected]/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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants