Add opt-out for JSON date sanitization with tests - #845
Conversation
since this was breaking conversion for datasets from bruker scanner
Co-authored-by: akhanf <11492701+akhanf@users.noreply.github.com>
Co-authored-by: akhanf <11492701+akhanf@users.noreply.github.com>
Co-authored-by: akhanf <11492701+akhanf@users.noreply.github.com>
yarikoptic
left a comment
yarikoptic
left a comment
There was a problem hiding this comment.
Thank you for approaching this! I left one main comment for changing the name and keeping it consistent, and dropping a change to .gitignore. Otherwise -- thank you, let's get it finished!
| help="Exclude dcmstack meta information in sidecar jsons.", | ||
| ) | ||
| parser.add_argument( | ||
| "--no-sanitize-jsons", |
There was a problem hiding this comment.
let's make it consistent sanitize_dates name since it is not as much about JSON (IIRC _scans.tsv might also be affected) but rather about date , and later we might add sanitization through offset (like recommended in BIDS, so let's make it not bool option but rather a mode of operation. Hence here
--sanitize-dates with default to be "remove", and you add "nothing" (or alike). And I (or someone) later might add some kind of "shift:". Then pass that variable around instead of differently named bool.
There was a problem hiding this comment.
Thank you for the feedback @yarikoptic! Just pushed a commit to make the suggested change -- would appreciate it if you could review it again, thanks!
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
…ange from type bool to str
yarikoptic
left a comment
yarikoptic
left a comment
There was a problem hiding this comment.
major one is absent "shift" implementation while listed.
Also, potentially we could/should keep old sanitize-json for compatibility reasons, just shimming into current mode of operation (remove) and potentially issuing a warning on better alternative available
| if sanitize == 'remove': | ||
| for f1 in ["Acquisition", "Study", "Series"]: | ||
| for f2 in ["DateTime", "Date"]: | ||
| json_.pop(f1 + f2, None) |
There was a problem hiding this comment.
what about other possible values? "nothing" should be explicitly handled and "shift" seems to be missing entirely.
For "shift" we also need to figure out how/where to pass such a secret value of the shift (likely via env var like HEUDICONV_DATETIME_SHIFT or alike, also it then should be ensured to be used while storing in _scans.tsv ).
while implementing "shift" there should be a check that it does shift beyond " shifted dates to the year 1925 or earlier." (https://bids-specification.readthedocs.io/en/stable/common-principles.html#units) and also that we do not shift if already shifted.
Summary
This PR introduces an explicit opt-out mechanism for JSON date sanitization, allowing users to preserve original metadata when needed while keeping existing sanitization behavior as the default.
Key changes
Adds a --sanitize-dates CLI option to control date sanitization behavior
Threads the sanitize_dates flag through the relevant function chain
Adds targeted tests for tuneup_bids_json_files to validate sanitization behavior
Cleans up related linting issues
Testing