action order on same field

[glebsts]

Hello,
I am confused by following in docs:

For the above, given that there are conflicting commands, the more conservative is given preference. For example:
REMOVE > BLANK > REPLACE > JITTER/KEEP/ADD
...
If I specify to blank a header and remove it, it will be removed
If I specify to replace a header and blank it, it will be blanked

Also

Most of the time, you won’t need to specify remove, because it is the default

But at the same time there is an example of

ADD PatientIdentityRemoved Yes
REMOVE ALL
KEEP PixelData

Here I naturally make assumption that ALL means ALL, so each field is removed, and according to rules quoted above, REMOVE rules over KEEP, so tag kinda has to be removed.
How does it work? How is PixelData expected to be kept?

In my usecase I want to drop all information except for certain tags, part of them I want to keep, part of them I want to replace with my custom func.
I don't want to manage full set of DICOM tags, but rather remove all and preserve only certain subset.
How?

[vsoch]

I would try to follow the example and do a REMOVE ALL and then add the fields you want to keep. If that is successful we can update the docs to reflect it - I think the spirit of the docs is that if you were to have all those commands in the same file for the same field, it's saying that REMOVE would take preference. But it also could be that they are out of date.

Please test it out for your use case and report back!

[glebsts]

I tested on

REMOVE ALL
KEEP SeriesDescription

and it ends up with everything removed. Same if I swap lines.
So seems like if I run REMOVE ALL, I can't REPLACE or KEEP any tag anymore.

So no idea how can I only concentrate on tags I need with dropping everything that I don't need with one-liner (aka whitelisting)?

Writing documentation for open-source project based on blackbox testing sounds really weird.

[vsoch]

ah then the documentation is correct. :)

Writing documentation for open-source project based on blackbox testing sounds really weird.

I don't understand this comment (it seems like it could be a bit insulting) but I'll just say that if you see an issue in the docs you are welcome to suggest changes. In this case the docs are correct, so there's that.

In practice it's not hugely practical to just wipe every header field from a dicom - many of them are needed. I would suggest you look at your data and come up with reasonable filters (e.g., removing everything with a date, or name, the library supports regular expression matching for that).

Insulting the maintainers of the library for their documentation is not going to get you very far. Just FYI. :)

[glebsts]

I'm sorry it it sounded insulting, just in my world documentation is based on actual code logic and is updated together with code changes, otherwise other teams depending on product will struggle. It is not exactly an open-source world, but I am trying to follow same practices when contributing into OSS :)

As for this specific example in docs:

ADD PatientIdentityRemoved Yes
REMOVE ALL
KEEP PixelData

which doesn't make too much sense (even just-added PatientIdentityRemoved is removed and not present in outcome), I can only suggest separate paragraph about REMOVE ALL behavior and dropping example.

Sadly my usecase is exactly about dropping everything except for tags needed to display image, and uuids required to distinguish between studies are being anonymized. The rule we got from healthtech guys from google is "use whitelists to avoid GDPR/etc problems". In total it makes about ~30 fields to be left in file. In one of example recipes I found there is 240 rows of tags being replaced - I just don't want to manage such a big list, if I could go with ~30 lines whitelist.

What do you think if I i.e. raise PR with whitelisting support? Not sure yet, how, if I would like to keep existing REMOVE ALL behavior for backwards compatibility. Some kind of switch, or totally new keyword like BLACKLIST ALL which can be overridden for tags by following regular actions?
So that order would become

REMOVE > BLANK > REPLACE > JITTER/KEEP/ADD > BLACKLIST

meaning that tag is removed unless any of existing actions is added to field (existing actions become whitelisting-actions).

Smth like that. Would it fit into deid roadmap?

[vsoch]

No worries @glebsts - sometimes it's hard to tell with communication on these boards.

Your use case is new, and I think we could definitely support it with the right tweaks, and it would also be good to fix that (now dated and not working) section of the docs.

Note that we do have a "protected" set of fields https://github.com/pydicom/deid/blob/master/deid/dicom/config.json that would hurt the integrity of the dicom file if removed, so if you see something not being removed that you would expect, take a look there.

We have whitelisting for the filter section of deid, so logically we could support some kind of the same thing for header fields. But I'm wondering can we handle this case just with the current tags? E.g., if REMOVE ALL and then a series of KEEP were to work, wouldn't that be sufficient? I ask because it would be messy in the format of the deid.dicom to have a header with both action statements and filters, especially when the action statements can already have regular expressions.

Let me think more about it - and feel free to write up a spec in the meantime. I'm going into a few hours of work and ping me again if I don't remember to respond!

[vsoch]

Oh and could you give me an example (with comments) about how a blacklist/whitelist action would work?

[glebsts]

I myself go to sleep now, it is midnight here in GMT+3 DTS :) Will get back to OSS in couple of days.
REPLACE ALL + KEEP to work would make totally sense, but I was thinking about keeping backwards compat. That would be a breaking change, and if set into major release, can be done as is. My BLACKLIST suggestion was only because of backwards compatibility. If it is not important, could just normalize behavior of replace+(keep|add|jitter|replace) combo.
That would just mean changing order to

BLANK > REPLACE > JITTER/KEEP/ADD > REMOVE

But I was not yet too deep in deid internals, not sure if I can first iterate whole recipe and build list of fields to keep and check it upon every remove action. Will see..

[vsoch]

To answer your first question - yes we would absolutely be open to a PR to make this change - looking forward to chatting in a few days when you are back!

[glebsts]

Hi there, quite long read for you - tests and my three statements and suggestions I'd like to implement. Vanessa, pls add your opinion on those before I start digging in code.

---

So I added some tests to test_replace_identifiers that illustrate minimal logic points that I'd like to clarify.

---

    def test_remove_all_keep_field_compounding(self):
        dicom_file = get_file(self.dataset)

        actions = [
            {"action": "REMOVE", "field": "ALL"},
            {"action": "KEEP", "field": "StudyDate"},
            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
        ]
        recipe = create_recipe(actions)

        parser = DicomParser(dicom_file, recipe=recipe)

        print(f"Before: {parser.dicom}")
        parser.parse()

        print(f"After: {parser.dicom}")

        self.assertEqual("Yes", parser.dicom['PatientIdentityRemoved'].value)
        self.assertIsNotNone(parser.dicom['PixelData'])
        self.assertEquals('20230101', parser.dicom['StudyDate'])

that produces following:

Before: Dataset.file_meta -------------------------------
# ... skipped lots of fields here
(0008, 0020) Study Date                          DA: '20230101'
(0008, 0070) Manufacturer                        LO: 'SIEMENS'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements

After: Dataset.file_meta -------------------------------
# [File Meta Information Group Length, File Meta Information Version, Transfer Syntax UID, Implementation Class UID]
# skipped here, they are preserved from original data
(0012, 0062) Patient Identity Removed            CS: 'Yes'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements

and fails with Error KeyError: (0008, 0020) (which is StudyDate)

---

    def test_remove_except_field_keep_other_field_compounding(self):
        dicom_file = get_file(self.dataset)

        actions = [
            {"action": "REMOVE", "field": "except:Manufacturer"},
            {"action": "KEEP", "field": "StudyDate"},
            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
        ]
        recipe = create_recipe(actions)

        parser = DicomParser(dicom_file, recipe=recipe)

        print(f"Before: {parser.dicom}")
        parser.parse()

        print(f"After: {parser.dicom}")

        self.assertEqual("Yes", parser.dicom['PatientIdentityRemoved'].value)
        self.assertIsNotNone(parser.dicom['PixelData'])
        self.assertIsNotNone(parser.dicom['Manufacturer'])
        self.assertIsNotNone(parser.dicom['ManufacturerModelName'])
        self.assertIsNotNone(parser.dicom['StudyDate'])

that produces following:

Before: Dataset.file_meta -------------------------------
# ... skipped lots of fields here
(0008, 0020) Study Date                          DA: '20230101'
(0008, 0070) Manufacturer                        LO: 'SIEMENS'
(0008, 1090) Manufacturer's Model Name           LO: 'SOMATOM Definition AS+'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements

After: Dataset.file_meta -------------------------------
# [File Meta Information Group Length, File Meta Information Version, Transfer Syntax UID, Implementation Class UID]
# skipped here, they are preserved from original data
(0008, 0070) Manufacturer                        LO: 'SIEMENS'
(0008, 1090) Manufacturer's Model Name           LO: 'SOMATOM Definition AS+'
(0012, 0062) Patient Identity Removed            CS: 'Yes'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements

and fails with Error self.assertIsNotNone(parser.dicom['StudyDate'])

---

    def test_remove_all_add_other_field_compounding(self):
        dicom_file = get_file(self.dataset)

        actions = [
            {"action": "REMOVE", "field": "ALL"},
            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
            {"action": "ADD", "field": "StudyDate", "value": "19700101"},
        ]
        recipe = create_recipe(actions)

        parser = DicomParser(dicom_file, recipe=recipe)
        print(f"Before: {parser.dicom}")
        parser.parse()

        print(f"After: {parser.dicom}")

        self.assertEqual("Yes", parser.dicom['PatientIdentityRemoved'].value)
        self.assertIsNotNone(parser.dicom['PixelData'])
        self.assertNotEqual("19700101", parser.dicom['StudyDate'].value)

that produces following:

Before: Dataset.file_meta -------------------------------
# ... skipped lots of fields here
(0008, 0020) Study Date                          DA: '20230101'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements

After: Dataset.file_meta -------------------------------
# [File Meta Information Group Length, File Meta Information Version, Transfer Syntax UID, Implementation Class UID]
# skipped here, they are preserved from original data
(0008, 0020) Study Date                          DA: '19700101'
(0012, 0062) Patient Identity Removed            CS: 'Yes'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements

and fails with AssertionError: '19700101' == '19700101'

---

Here I have following statements (order not related to tests):

1. ADD tag is working with REMOVE, adding new tag after everything has been dropped, even same tag. Not exactly behavior of REMOVE > ADD rule in case of existing tag (kinda mimics REPLACE), but it is a behavior I would actually expect when reading the recipe. I am not sure if I want to change behavior, because it works for me, but I would clarify that in docs by removing ADD from REPLACE > ..... > KEEP/ADD rule.
2. KEEP tag is not working with REMOVE tag, REMOVE except:tag or REMOVE ALL, and if I want to keep StudyDate, I can't do it by whitelisting it.
   I assume semantically REMOVE ALL followed by KEEP tag is expected to keep tag. It doesn't. I'd like to change deid to support such flow.
3. REMOVE tag/REMOVE except:tag is working with substrings (didn't add test above, but if you make REMOVE except:Model, it keeps ManufacturerModelName. I find this place slightly unclear in docs, except section of docs says "list of field", while it is actually regex per se. There are examples with explicit contains:Name also in docs, but lib acts as it is implicit. I'd like to clarify that by updating except documentation, though might miss it in some other place.

[vsoch]

Thank you for the detailed write-up - here are my thoughts so you can move forward:

ADD tag is working with REMOVE, adding new tag after everything has been dropped, even same tag. Not exactly behavior of REMOVE > ADD rule in case of existing tag (kinda mimics REPLACE), but it is a behavior I would actually expect when reading the recipe. I am not sure if I want to change behavior, because it works for me, but I would clarify that in docs by removing ADD from REPLACE > ..... > KEEP/ADD rule.

I agree - I would remove the REMOVE > ADD tag from the docs, as I believe it reflects the original implementation of deid where we parse over actions first, decided on some "final" state and then proceeded. This version moves through the rules top to bottom and essentially follows them, so it makes sense the user can ADD, REMOVE, ADD, REMOVE in infinitude. It indeed does mimic replace, at least with that sequence of actions. I don't think we should change this behavior, but should just fix that section in the docs.

KEEP tag is not working with REMOVE tag, REMOVE except:tag or REMOVE ALL, and if I want to keep StudyDate, I can't do it by whitelisting it.

If ADD/REMOVE can be thought of as changing with state, KEEP would be akin to a whitelist, as to say "no matter what happens, do not remove this tag." And that probably means it is allowed to be edited (e.g., KEEP would work with REPLACE or ADD but just not remove). The easiest way to implement this would be to parse KEEP first, and then ensure if something is to be REMOVE it is not in the KEEP set.

I assume semantically REMOVE ALL followed by KEEP tag is expected to keep tag. It doesn't. I'd like to change deid to support such flow.

I agree, and probably in the docs we would want to stress that most actions are done based on current state, but keep is more global (and thus doesn't matter where it's used).

REMOVE tag/REMOVE except:tag is working with substrings (didn't add test above, but if you make REMOVE except:Model, it keeps ManufacturerModelName. I find this place slightly unclear in docs, except section of docs says "list of field", while it is actually regex per se. There are examples with explicit contains:Name also in docs, but lib acts as it is implicit. I'd like to clarify that by updating except documentation, though might miss it in some other place.

I would absolutely love an improvement to the docs here! I likely documented the pattern to use a regex but not in context of a particular tag.

[glebsts]

Thank you for comments.

I agree, and probably in the docs we would want to stress that most actions are done based on current state, but keep is more global (and thus doesn't matter where it's used).

Could you please explain what do you mean by "based on current state"?

In general I take that as "Yes" and will proceed with implementing whitelists (aka "KEEP overrules REMOVE") and updating docs.

[vsoch]

Could you please explain what do you mean by "based on current state"?

Sure! So imagine that we have a box with different colored balls (this is the dicom file, and each ball is a header entry). If we then have a list of actions, we can move down the list one by one and interact with the balls. E.g.,:

• REMOVE red # the new state is that the box has no red balls (state 1)
• ADD red # the box has a red ball again (state 2)
• REMOVE red # again they are gone, we are back to state 1

So those are stateful actions because using them at some timepoint will change the state of what is in the box. So KEEP would be a global command (not stateful) so if you did the same:

• KEEP red # a global statement that says "freeze the state of the box so we always keep red balls, I don't care what stateful actions come after)
• REMOVE red # we don't remove any red, we have the same state
• ADD red # again, no change

So (just generally) when I'm comparing actions like ADD/REMOVE I'm thinking of those as stateful because they are scoped to a specific point in time. A KEEP, however, is not specific to a point and time - no matter where you use it in your recipe it is going to "pin" some global state. Does that kind of make sense?

[glebsts]

Yes, thank you, nice example :) I'll think how to put it into words without using balls, but saying in advance - feel free to patch my wording, as you seem to be native speaker :)

[vsoch]

Haha definitely! It's probably not the best example - they always used colored balls in bags as examples in statistics courses, so I'm not sure I did it justice 😆

[glebsts]

Hi there,
I pushed PR with quite straightforward solution (and tests). Before I proceed with updating all the docs (I'm slightly scared by amount of it), I'd like to know your opinion about approach taken.

[vsoch]

I think the approach is fairly straight forward - the one issue I see is that self.skip is actually a property, so I'm not sure it makes sense to append to that. Perhaps that logic should be moved into the property?

Also - are we sure that KEEP will only be relevant for a field? E.g., we are just appending the KEEP field to skips. What if there is a more fine tuned logic?

[glebsts]

skip is indeed property, and underlying getter should return ref to actual underlying instance variable, append is in-place operation. Seemed good enough for me, and tests shows it is valid (I also inspected during debug).. Skiplist is only checked in case of REMOVE, ADD doesn't care about it.. Semantically fields to KEEP we would like to skip from REMOVE. Did you mean introducing completely new property, which usage will be identical to skip?

As for "more logic" I could only come up with some expansion on KEEP, but imo for whitelisting it is better to have it to be explicitly specific field names. I.e. skiplist config only supports list of fields.

[vsoch]

It functionally works, but if someone wants to access the consistent list of things to skip, it doesn't make sense to have custom logic separate in the parse() function and then essentially return a different answer. This way also doesn't allow the disable_skips attribute to apply to the current recipe. So I would do:

    @property
    def skip(self):
        """
        Return a list of fields to skip, as defined in the self.config
        """
        skips = []
        if self.disable_skips:
            return skips

        if self.config:
            skips = self.config.get("get", {}).get("skip", {})
        if self.recipe.deid is not None:
            for action in self.recipe.get_actions(action="KEEP"):
                skips.append(action.get("field"))
        return skips

With the current functionality we:

1. are not adding these to skip unless its parsed
2. if it is parsed multiple times we add multiple instances to skip
3. we are not honoring the variable to not use the skip list.

Let me know if there is anything you want to discuss.

[glebsts]

Thank you for well-structured answer :)
I agree on consistency and other points you've brought. Haven't using disable_skips myself, I didn't think about it.
Looking at suggested getter code, I have a bad feeling about disable_skips vs KEEP.
Imo (disable_skip: true) != (ignore KEEP actions)
I.e. I want to disable skips (i.e. deleting PixelData), and REMOVE ALL and KEEP StudyDate.
That should remove absolutely everything, but keep StudyDate.
In such situation I might even find a logically more transparent solution of creating another property fully similar to skip but not respecting this disable_skips parameter. Basically duplicating all code pieces using skip in perform_action also check for remove if not in keep-list

 if contender.keyword in skip or str(contender.tag) in skip 
    or contender.keyword in keep or str(contender.tag) in keep:
         continue

(might be slightly shortened, but logically same)

Would like to hear your opinion on that :)

[vsoch]

That's a good point - perhaps we should have two properties then? self.skip remains as it is now, and then self.keep could be the similar functionality, but run the logic that you have now in the parse function. That way, we'd have the logic separate but still not be appending to a property, and the other issues I raised. E.g.,

@property
def keep(self):
    keeps = []
    if self.recipe.deid is not None:
        for action in self.recipe.get_actions(action="KEEP"):
            keeps.append(action.get("field"))
    return keeps

And then in the function call to get_fields just include both of them:

def get_fields(self, expand_sequences=True):
        """expand all dicom fields into a list, where each entry is
        a DicomField. If we find a sequence, we unwrap it and
        represent the location with the name (e.g., Sequence__Child)
        """
        if not self.fields:
            self.fields = get_fields(
                dicom=self.dicom,
                expand_sequences=expand_sequences,
                seen=self.seen,
                skip=self.skip + self.keep,
            )
        return self.fields

as including the list of keeps as skips does make sense for applying the keep logic!

[glebsts]

yes, makes sense.. I'll check disable_skips usage and go the way you've suggested soon, together with docs update

[glebsts]

@vsoch you are welcome to approve (or not approve :)) and merge

[vsoch]

Closed with #198