[vamsibvv]

Description

Related issues: #274

REPLACE, JITTER and REMOVE now raise a ValueError when their var:, func: or deid_func: isn't available, instead of skipping the field.

Skipping kept the original value. With

%header
REMOVE ALL
REPLACE PatientID var:new_id

and no new_id for a file, the original PatientID is written out, because REPLACE fields are kept by REMOVE ALL. A missing var: doesn't log anything either. The same happens with func:, deid_func:, JITTER, and REMOVE ALL func:... (nothing gets removed).

parse_value takes required=False, and REPLACE, JITTER and REMOVE pass required=True. ADD still skips, since the default recipe has ADD PatientID var:entity_id and similar lines that most people don't provide. A REPLACE for a field the file doesn't have doesn't raise either, because the action never runs.

In replace_identifiers, a file with a missing value is logged and skipped (not saved or returned), the other files are still processed, and a MissingLookupError (a ValueError) naming every skipped file is raised at the end.

This changes behavior for recipes that relied on the skip, so I added a CHANGELOG entry (0.4.14) and a note in recipe-headers.md. If you'd rather make it opt-in, for example with a strict argument, I can change it.

To test: python -m unittest deid.tests.test_dicom_funcs. The new tests cover the five cases above, replace_identifiers (raises and writes no file, and works when the variable is given), ADD still skipping, and a batch where one file has no value (only the other file is written, and the error names the skipped one). The first two fail on master. Full suite: 284 tests, only test_cli and test_utils error on my Windows machine (temp files still open during cleanup), same as on master.

Checklist

• I have commented my code, particularly in hard-to-understand areas
• My changes generate no new warnings
• My code follows the style guidelines of this project

Open questions

Raise by default vs. opt-in, as above.

AI assistance (Claude Code) was used for this change.

[vsoch]

Overall looks good! One comment and item for discussion below.

[vsoch]

Let's add the field and file basename here as well.

My one concern about this is that a single missing value is going to raise error for the entire batch. That means you would get partial writes.

[vamsibvv]

Added in e1eb17d, e.g. var:new_name not found in item lookup. Field: PatientName, file: ctbrain1.dcm.

On the batch: replace_identifiers could catch the error per file, log it, skip writing that file and continue with the rest, then raise at the end with the files that failed. Would that work for you?

[vsoch]

I think that would be more ideal than stopping mid-batch without any awareness for what has been done. Others can chime in if they have opinion.

[vamsibvv]

Done in 9abb594. replace_identifiers now logs and skips a file with a missing value (it is not saved or returned), processes the rest, and raises at the end with the skipped files, e.g. 1 of 2 files were skipped because of a missing lookup: var:new_name not found in item lookup. Field: PatientName, file: ctbrain2.dcm.

Separately, CI on this PR fails at conda create with conda: command not found on the runner, before any of the code runs.

[vsoch]

Yes - that issue has been hitting many of the other repos I maintain too! The old ubuntu runner came with conda, and it was removed. I'll see if I can put in a fix you can rebase to.

[vsoch]

If missing is not empty, what do we get by raising an error here? If save is True, the subset of updated files are updated (but we never return them to the user). If save is False, we lose those files.